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

Add concept exercise conditionals #1037

Merged
merged 25 commits into from
Mar 16, 2021

Conversation

junedev
Copy link
Member

@junedev junedev commented Mar 13, 2021

Addresses #989

@SleeplessByte
Copy link
Member

Exciting stuff!

Once you undraft this and fix the conflicts and still run into prettier, you can add a comment /format as the only content of that comment to trigger prettier here:

/format

The same goes for integrity issues. If the precheck fails and complains that the files are not in sync, you can use /sync as the only content of a comment to have a GitHub Action sync all the exercises:

/sync

@exercism exercism deleted a comment from github-actions bot Mar 14, 2021
@exercism exercism deleted a comment from github-actions bot Mar 14, 2021
@exercism exercism deleted a comment from github-actions bot Mar 14, 2021
@exercism exercism deleted a comment from github-actions bot Mar 14, 2021
@exercism exercism deleted a comment from github-actions bot Mar 14, 2021
@exercism exercism deleted a comment from github-actions bot Mar 14, 2021
@SleeplessByte
Copy link
Member

Okaaaaay. It wasn't supposed to trigger it, but 🤷🏽 . Luckily everything was already up to date, so nothing happened 🌠

@junedev junedev changed the title [WIP] Add concept exercise conditionals Add concept exercise conditionals Mar 14, 2021
@junedev junedev marked this pull request as ready for review March 14, 2021 13:59
@junedev junedev requested a review from a team March 14, 2021 13:59
@junedev
Copy link
Member Author

junedev commented Mar 14, 2021

Open questions

  1. Scope: Currently the ternary operator is out of scope for this exercise. It think it is better to teach it somewhat later because otherwise students might overuse it and create code that is hard to read but makes them feel clever. Should we keep it like it is or include the ternary operator?
  2. Concept naming: Currently the concept for the if statement is called conditionals. Since there will also be conditionals-switch and conditionals-ternary later on, conditionals-if would be a more consistent name. Should I stick with conditionals nevertheless?
  3. About.md content: I included some parts (e.g. on how to write the condition) that are more "general good practice" than JavaScript specifics. I am of course open to removing some or all of those if they are unwanted. Just let me know.
  4. Enforcing if/else if/else: In these simple functions we are dealing with in the tasks you could always also use early returns instead of practicing how to write else if and else what is the goal of the exercise. I have dealt with that by tweaking the exercise such that not using early returns avoids some duplication and by explicitly mentioning what should be used in case of task 3. Additionally I understand our analysis tooling can also help here. Any other thoughts on how to deal with this issue?

Note

I removed the details about loose equality from the comparison page because as discussed in Slack, a student does not need to know them to write good JS code.
@SleeplessByte I thought by writing "We should slash the entire table" in Slack you agreed to that but then I saw you moved it instead of "slashing" it. 🤔

@SleeplessByte
Copy link
Member

SleeplessByte commented Mar 14, 2021

I moved it so you could decide :)

I'll respond to the rest later 🤗

@SleeplessByte
Copy link
Member

SleeplessByte commented Mar 15, 2021

Scope: Currently the ternary operator is out of scope for this exercise. It think it is better to teach it somewhat later because otherwise students might overuse it and create code that is hard to read but makes them feel clever. Should we keep it like it is or include the ternary operator?

I would even suggest that it's okay if we don't explicitly teach the ternary operator. Its use is often not necessarily more idiomatic, but a stylistic choice. I feel very (positively) strongly about using it, but I know that various style guides discourage or even ban it altogether. Does that mean we should skip it? No, but it does mean its not a worthy concept to put our time in right now ;).

I think it's a good idea to leave it out of this exercise and revisit it some time in the future when we've covered the other bases.

Concept naming: Currently the concept for the if statement is called conditionals. Since there will also be conditionals-switch and conditionals-ternary later on, conditionals-if would be a more consistent name. Should I stick with conditionals nevertheless?

Eh. Yes. I agree with you, but we have made the choice that the "basics of x" / "x 101" / "introduction to x" is named "x", and not "x-something". I think this is a great exercise to teach people how to do the majority of conditionals, and thus I think conditionals is aptly named. In fact, any exercise that potentially expects a ternary or switch can probably be solved by someone who has only done this exercise. My vote would be conditionals for cross-track consistency.

About.md content: I included some parts (e.g. on how to write the condition) that are more "general good practice" than JavaScript specifics. I am of course open to removing some or all of those if they are unwanted. Just let me know.

I like this. Let's keep it.

Enforcing if/else if/else: In these simple functions we are dealing with in the tasks you could always also use early returns instead of practicing how to write else if and else what is the goal of the exercise. I have dealt with that by tweaking the exercise such that not using early returns avoids some duplication and by explicitly mentioning what should be used in case of task 3. Additionally I understand our analysis tooling can also help here. Any other thoughts on how to deal with this issue?

I often would write early returns. We can definitely have the Analyzer check for lack of else statements and then print out something along the lines of:

Nice. 
That's an _early return_. 
For the purpose of the Concept that this exercise aims to teach, try solving this using an `else` statement.

If you'd like something like this (you can suggest the copy), can you add it to design.md at the bottom? See lasagna if you want to know what I mean. I can then point you / aid you / do this for you in the Analyzer. Let me know.

Copy link
Member

@SleeplessByte SleeplessByte left a comment

Choose a reason for hiding this comment

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

A few typos, and one change request: the tests themselves. Otherwise this is perfect.

Feel free to apply which you like, and then self squash merge.

concepts/comparison/about.md Show resolved Hide resolved
concepts/comparison/about.md Outdated Show resolved Hide resolved
concepts/comparison/about.md Outdated Show resolved Hide resolved
concepts/comparison/about.md Outdated Show resolved Hide resolved
concepts/comparison/about.md Outdated Show resolved Hide resolved
exercises/concept/vehicle-purchase/.docs/instructions.md Outdated Show resolved Hide resolved
exercises/concept/vehicle-purchase/.docs/introduction.md Outdated Show resolved Hide resolved
exercises/concept/vehicle-purchase/.docs/introduction.md Outdated Show resolved Hide resolved
exercises/concept/vehicle-purchase/.meta/design.md Outdated Show resolved Hide resolved
Copy link
Member

@kotp kotp left a comment

Choose a reason for hiding this comment

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

Some little suggestion(s).

concepts/comparison/about.md Outdated Show resolved Hide resolved
concepts/comparison/about.md Outdated Show resolved Hide resolved
config.json Show resolved Hide resolved
@junedev
Copy link
Member Author

junedev commented Mar 16, 2021

@SleeplessByte
I added the note regarding the Analyzer feedback to the design.md file.

Feel free to apply which you like, and then self squash merge.

I fixed all the things mentioned in the review but I don't have rights to merge myself.

@SleeplessByte SleeplessByte merged commit 7f63dfd into exercism:main Mar 16, 2021
@SleeplessByte
Copy link
Member

SleeplessByte commented Mar 16, 2021

🎆🔥🚒🧯

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

Successfully merging this pull request may close these issues.

5 participants