Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update Diamond #2506

Open
wants to merge 9 commits into
base: main
Choose a base branch
from
Open

Update Diamond #2506

wants to merge 9 commits into from

Conversation

iHiD
Copy link
Member

@iHiD iHiD commented Dec 3, 2024

Changes:

  1. Tweak initial wording
  2. Start with examples - it makes it much clearer
  3. Remove middots to represent spaces and just use spaces.

I'd ideally like to get this merged ASAP as it's the featured exercise this week, but I'm very open to improvements/corrections! :)

@iHiD iHiD requested a review from a team as a code owner December 3, 2024 05:27
Copy link
Member

@ErikSchierboom ErikSchierboom left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving with one suggestion

exercises/diamond/description.md Outdated Show resolved Hide resolved
Copy link
Contributor

@Cool-Katt Cool-Katt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds to me a bit more clear this way. ¯_(ツ)_/¯

exercises/diamond/description.md Outdated Show resolved Hide resolved
@tasxatzial
Copy link
Member

3. Remove middots to represent spaces and just use spaces.

Is there any specific reason behind this change? With dots, it's easy to see the spacing between letters. Not so much with spaces.

@tasxatzial
Copy link
Member

tasxatzial commented Dec 3, 2024

Also, this might be a good opportunity for a small story?


In a quiet corner of the ancient Library of Patterns, you discover the Codex Diamantum, an artifact whispered about in legends. As you open it, the first page reveals a trial known to test skill, focus, and creativity: creating a perfect diamond shape from letters.

Each layer must grow outward to the middle, then shrink back in, forming a balanced and flawless pattern. The letters must align perfectly, their symmetry reflecting precision and care. Many have tried and failed, but those who succeed are said to uncover the codex's deeper mysteries.

The air around you feels alive with anticipation. This is no ordinary task — this is your chance to prove yourself worthy of the codex's secrets. Will you rise to the challenge and craft a diamond that stands as a testament to your mastery?


Edit: Codex Diamantum is in italics

https://forum.exercism.org/t/a-story-for-the-diamond-exercise/14036

Co-authored-by: András B Nagy <20251272+BNAndras@users.noreply.github.com>
@iHiD
Copy link
Member Author

iHiD commented Dec 3, 2024

Is there any specific reason behind this change? With dots, it's easy to see the spacing between letters. Not so much with spaces.

Yes. I thought you had to use dots to solve it when I looked at the images. As this is monospace, I think the spaces are clear enough to show what's happening.

@iHiD
Copy link
Member Author

iHiD commented Dec 3, 2024

Also, this might be a good opportunity for a small story?
Yes, I'd love that. But as a follow up PR please! :)

@tasxatzial
Copy link
Member

Yes. I thought you had to use dots to solve it when I looked at the images

This could have easily been solved with an explanation:

In the following examples, spaces are shown using · characters for clarity, but in the final shape, they are actual spaces.

@iHiD
Copy link
Member Author

iHiD commented Dec 3, 2024

This could have easily been solved with an explanation:

That was already there. And I missed it.

I think the exercise is pretty obvious from the diagrams, without really needing to read through all the bullet points. So making someone read them (and other details like spaces -> dots) to clarify the diagram (because the diagrams aren't actually representative of the output) feels like we're asking people to do unnecessarily work to me.

@Cool-Katt
Copy link
Contributor

I think the exercise is pretty obvious from the diagrams, without really needing to read through all the bullet points. So making someone read them (and other details like spaces -> dots) to clarify the diagram (because the diagrams aren't actually representative of the output) feels like we're asking people to do unnecessarily work to me.

I second this, I have to admit I only glanced at the bullet points so I could have easily missed this.

@tasxatzial
Copy link
Member

That was already there. And I missed it.

I'm genuinely confused. The examples are only meant to illustrate the shape of the diamond. Isn't the first thing anyone will do to look at the tests in order to see the actual requirements? Won't they immediately notice that there are no dots?

I think the exercise is pretty obvious from the diagrams, without really needing to read through all the bullet points.

The bullet points are another part of the exercise that, to me, seem to try too hard to explain the requirements. Personally, I'm not sure how I feel about reading 12 lines of instructions.

@Cool-Katt
Copy link
Contributor

Isn't the first thing anyone will do to look at the tests in order to see the actual requirements? Won't they immediately notice that there are no dots?

You'd be surprised at the amount of people that wouldn't ever consider reading the tests...

Comment on lines +42 to +43
- The diamond has a square shape (width equals height).
- The letters form a diamond shape.
Copy link
Member

@tasxatzial tasxatzial Dec 3, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The letters form a diamond shape, and the diamond has a square shape? This is unclear. The square shape refers to what?

Copy link
Member

@tasxatzial tasxatzial Dec 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see where my confusion came from. The diamond is introduced before explaining what it refers to. Might be better to simply reverse the order of the first two requirements

Edit: Removing the unneeded repetition of the word 'shape' may also be preferable since the requirements seem to suggest that we have both a diamond shape and a square shape

Suggested change
- The diamond has a square shape (width equals height).
- The letters form a diamond shape.
- The letters form a diamond shape.
- The width of the diamond is equal to its height.

Copy link
Member

@tasxatzial tasxatzial left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Despite the ongoing discussion and suggestions, I've decided that this is good enough for approval. Given the nature of the problem, I believe there will always be some ambiguity in how the verbal instructions are interpreted, so I'm not going to stress too much about the choice of words. The instructions are mostly fine as they are, but due to the challenge of articulating the exercise in words, I don't believe people will find them particularly helpful. I expect most people will simply refer to the visual diagram. Moreover, it's impossible to solve the exercise without looking at the tests. Therefore, as long as there are no contradictions in the current instructions, I'm okay with it.

Approved with suggestion (see prev review)

Copy link
Contributor

@Cool-Katt Cool-Katt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suppose it's better to agree that it's fine as it is, and argue about wording later on the forum.

@tasxatzial
Copy link
Member

@Cool-Katt It might be better to wait until Jeremy comes along. Perhaps the wording is something that does concern him.

@Cool-Katt
Copy link
Contributor

@tasxatzial
I'm definitely not yoloing with a merge, I just want to make the damn CI pass but apparently it's not happening.

@glennj
Copy link
Contributor

glennj commented Dec 4, 2024

The markdown linter doesn't like the trailing spaces in description.md

https://github.com/exercism/problem-specifications/actions/runs/12168211336/job/33938555168#step:6:196

@tasxatzial
Copy link
Member

tasxatzial commented Dec 5, 2024

I just want to make the damn CI pass but apparently it's not happening.

Yeah, I was referring to that. Jeremy might come along and wipe everything out, depending on how he decides to handle this. I've come across exercises that directly point to the test suite from the description instead of trying to explain the requirements. I believe the same approach could work here. Even if the requirements are completely removed, I doubt most people would have any trouble with implementation. Based on the source link in the metadata, this seems to be treated as a TDD exercise.

Edit: Then again, if the requirements are removed, i believe this could create conflict with the intro comment in the canonical data, but i'm not sure.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants