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

csharp/leap: Improve mentor notes #640

Merged
merged 1 commit into from Dec 4, 2018

Conversation

Projects
None yet
4 participants
@SleeplessByte
Contributor

SleeplessByte commented Dec 2, 2018

Add suggestions and talking points around the order of operation and evaluation. The old reasonable solution is logically incorrect!

csharp/leap: Improve mentor notes
Add suggestions and talking points around the order of operation and evaluation. The old reasonable solution is logically incorrect!

@iHiD iHiD requested a review from ErikSchierboom Dec 2, 2018

}
```
### Common suggestions
- Instead of using several, potentially nested if-statement, suggest to remove the if-statements by returning a single expression using the boolean `&&` and `||` operators.
- Instead of explicitly returning `true` or `false` (e.g. `if (x) return true else return false`), suggest to simply return the boolean condition (`return x`).
- If students are doing more than three checks in their if-statement, ask if they could get it down to just three checks. You could give them a hint to following the instructions in the README.
- If students are doing more than three checks in their if-statement, ask if they could get it down to just three checks. You could give them a hint to following the instructions in the README. There are only two cases that return true:
- a year MUST be a multiple of four AND NOT be a multiple of 100

This comment has been minimized.

@ErikSchierboom

ErikSchierboom Dec 3, 2018

Member

I like that you make this explicit.

- If students are doing more than three checks in their if-statement, ask if they could get it down to just three checks. You could give them a hint to following the instructions in the README. There are only two cases that return true:
- a year MUST be a multiple of four AND NOT be a multiple of 100
- a year MUST be a multiple of 400 (implies multiple of 4 and 100)
- The _order of operation_ and _evaluation_ matter, see talking points:

This comment has been minimized.

@ErikSchierboom

ErikSchierboom Dec 3, 2018

Member

Should this really be a common suggestion? To me, optimizing which checks are executed is nice, but not required at this point in the track (leap being the second exercise). Maybe this section could be moved as a whole to the talking points section?

@robkeim @jpreese what do you think?

This comment has been minimized.

@SleeplessByte

SleeplessByte Dec 3, 2018

Contributor

Substitute this simple exercise with very expensive function calls and suddenly we care. Teach the student to care now so they don't mess up later.

I think that was the original thought behind the note in the ruby and later JavaScript tracks.

This comment has been minimized.

@robkeim

robkeim Dec 3, 2018

I think whether or not this should be discussed depends on the student's previous experience.

For someone that is not new to programming but only new to the language it's a good thing to discuss when reviewing the solution.

However, for someone that is just getting started with programming I think that we need to be careful not to overwhelm them. Agreed that order of execution is an important thing to consider, but if you've just completed HelloWorld and this is your second exercise it's not the most important thing for you to be focusing on at that stage IMO.

This comment has been minimized.

@SleeplessByte

SleeplessByte Dec 4, 2018

Contributor

I definitely don't disagree with that. I think two-fer could be between this and hello world.
The issue still is that the experience is different between tracks in a, imo, destructive way.

Would it work if I made these their own topic, but not "forced suggestions"? Instead something the mentor can mention when they approve a solution without parentheses or "wrong" order? I understand that not all tracks need be the same, even though it feels wrong to me.

This comment has been minimized.

@ErikSchierboom

ErikSchierboom Dec 4, 2018

Member

For someone that is not new to programming but only new to the language it's a good thing to discuss when reviewing the solution.

Well, this is the target audience of Exercism: people that already know how to program but want to become fluent in a language.

```csharp
return year % 400 == 0 || (year % 4 == 0 && year % 100 != 0);
```
Albeit correct in output, the check `year % 400` is _only_ true for `1.03%` years that are divisible by 4. Always try to put the common path first. Logically, following the instructions putting this check first is **incorrect**.

This comment has been minimized.

@ErikSchierboom

ErikSchierboom Dec 3, 2018

Member

Why is inversing the test order logically incorrect? It is still the same logic, albeit with redundant checks.

This comment has been minimized.

@SleeplessByte

SleeplessByte Dec 3, 2018

Contributor

See reply below, logically here means the logic comprehension from the instructions. Could probably use a reference to it in the mentoring notes.

@SleeplessByte

This comment has been minimized.

Contributor

SleeplessByte commented Dec 3, 2018

This is matching the other tracks where exercises are being disapproved if they don't match the order of the instructions due to the redundancy of tests.

I've been mentored here that I could leave out parentheses which I didn't like as it is logically not the same solution; other students following multiple tracks might actually get confused if one track enforces this (JS
and Ruby do) and some tracks don't and if fact discourage it (csharp and TypeScript at the moment, PRs are open).

Personally I think the exercise was chosen, originally for the ruby track, exactly because it's simple to implement but has a lot of haken en ogen.

I can't use GitHub formatting in email response, but the second comment
about why it's not logically correct is because it's not:

The instructions are as follows +paraphrased+

A year is a leap year if it is divisible by 4 AND

  • it is not divisible by 100
  • OR it is divisible by 400

The inverse test or lacking parentheses yield different "resulting instructions". This sets up the student to read the instructions carefully.
None of the tests of none of the languages test for implementation details (yay) so this is meant to set them up to receive comments about skimming instructions instead of carefully implementing them.

@ErikSchierboom

After some thought, I'll approve this. It's up to the mentor to determine if the student could benefit from pointing out the execution order problem.

- If students are doing more than three checks in their if-statement, ask if they could get it down to just three checks. You could give them a hint to following the instructions in the README. There are only two cases that return true:
- a year MUST be a multiple of four AND NOT be a multiple of 100
- a year MUST be a multiple of 400 (implies multiple of 4 and 100)
- The _order of operation_ and _evaluation_ matter, see talking points:

This comment has been minimized.

@ErikSchierboom

ErikSchierboom Dec 4, 2018

Member

For someone that is not new to programming but only new to the language it's a good thing to discuss when reviewing the solution.

Well, this is the target audience of Exercism: people that already know how to program but want to become fluent in a language.

@ErikSchierboom ErikSchierboom merged commit 11680ae into exercism:master Dec 4, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@SleeplessByte

This comment has been minimized.

Contributor

SleeplessByte commented Dec 4, 2018

Thanks!

FWIW I am happy to restructure this after a while if some things prove not to be as effective as I hope them to be.

@ErikSchierboom

This comment has been minimized.

Member

ErikSchierboom commented Dec 4, 2018

FWIW I am happy to restructure this after a while if some things prove not to be as effective as I hope them to be.

Let's keep it pragmatic and do just that! Thanks for the PR!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment