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

Ruby/HighScores: New Notes Format #633

Merged
merged 2 commits into from Dec 11, 2018

Conversation

Projects
None yet
7 participants
@F3PiX
Contributor

F3PiX commented Nov 30, 2018

Here's Mentor Notes with a different format.

It's meant to make mentoring easier for mentors, and also improve consistency of mentoring.

  • List the new concepts covered by the exercise
  • Help mentors decide to approve a solution or not
  • Help mentors that are fairly new to the language to mentor to idiomatic solutions
  • Help mentors to keep track of possible approaches and the discussions on those
  • List the collected wisdom of research notes
    and in general improve mentor happiness.

Note on the new 'Minimal Solution for Approval' : this is meant for the easiest exercises only, where there's only one, maybe two idiomatic solutions. That goes for only a few exercises. In Ruby: TwoFer, Acronym, HighScores.

@iHiD iHiD self-requested a review Nov 30, 2018

@pgaspar

Love the new format 👍 Thank you!

### Reasonable approaches
- See the one above
- Defining `@scores`, `@personal_best` and `personal_top` in the constructor.

This comment has been minimized.

@pgaspar

pgaspar Dec 2, 2018

Member

Not sure I love this approach being here. It's very brittle and breaks as soon as the scores array is modified after instantiation (even though we don't test for this scenario). Would love to hear your thoughts!

This comment has been minimized.

@F3PiX

F3PiX Dec 2, 2018

Contributor

I just went with the fact that one of the other mentors approved this approach, to be honest.
You're totally right that it would break after the scores are modified, but the thing is: we don't test that, and that's for a reason. And I don't like to reject solutions based on reasons that we don't test for. In pure TDD mode, this would suffice to get the tests to pass.
I would be very happy if there's another argument why this is not a good approach.

This comment has been minimized.

@pgaspar

pgaspar Dec 2, 2018

Member

And I don't like to reject solutions based on reasons that we don't test for.

🤔 so do you always accept as soon as the tests pass? There's a lot of idiomatic nuance we can't realistically test for. This is a broader discussion, but from what I've read before I think a lot of mentors require more from a solution than just passing the tests, no?

For example, in Acronym I typically point to using scan when people use split as suggested in the mentor notes. Do you usually accept split solutions and then suggest scan or only accept when scan (or something in that spirit) is used?

That's why I'm mentioning this - I'd be ok with someone with more experience using this approach (after making sure they understand what it means), but I'm afraid it can lead to confusion for novice programmers. So I would always follow an approach like this with a discussion on if and why it may be troublesome for you in the future. And I wouldn't recommend it for this exercise.

This comment has been minimized.

@SleeplessByte

SleeplessByte Dec 4, 2018

Contributor

I agree with @pgaspar , even though it's not tested for, a brittle solution is not a good guide for a mentor. If your example here is that "ok but TDD, this is barely enough", then why don't we accept shameless green solutions (solutions that basically have hardcoded solutions to the test cases)? However: I completely agree that solutions should not be rejected if not covered by the instructions or test cases. Note here that IMO the instructions are part of the challenge and should not be disregarded.

Example: A mentor on a different track mentored my leap exercise, suggesting I remove the parentheses, because their minimal solution did not have it in the guide. I had to explain that logically the solution to leap without parentheses is not equivalent.

This comment has been minimized.

@F3PiX

F3PiX Dec 4, 2018

Contributor

There's two questions in here:

  1. Do we want brittle/non-idiomatic solutions to be approved, just because they pass the tests?
  2. Is the 'everything in the initializer' an approvable approach?

  1. No.
  2. I think not, I wouldn't do it myself, but why? If the only argument against it is that it will break if we'd add new functionality, I don't find that very convincing.
    If we think it's an approach that shouldn't be approved, or should be approved with food for thought, I'd like to add the arguments here that can help new mentors.

Four possible solutions in this matter, in the order of my liking:

  1. Add a test that proves this to be a brittle/bad solution.
  2. An argument other than 'it will break if we add new functionality'.
  3. Add some explanation in the Mentor Notes here, that we can not not approve this, due to the limitations of the exercise, so do approve with food for thought , including a recommendation to extract methods.
  4. Add text to the instructions that implies that putting everything in the initializer is not an acceptable solution here.

What do you all think?

This comment has been minimized.

@SleeplessByte

SleeplessByte Dec 4, 2018

Contributor

Hi Maud,

I think you're asking the right questions!! (sidenote: in GFMD, you have to add an extra newline if you want a new list, now it show's up like 1 2 3 4. Typing 1. 1. 1. 1. would have resulted in the same).

I actually think that the FOUR solutions you provide should all / or at least the first three be implemented

Add a test that proves this to be a brittle/bad solution.

If this is a possibility then yes. However, forcing high scores to be immutable, would also fix this issue. Testing that could be:

  • init
  • modify scores array outside of the class
  • check if the outcome reflects the initial values

An argument other than 'it will break if we add new functionality'.

My argument is the Example I listed above. A "bad" minimal solution will have mentors use that as baseline.

Is the 'everything in the initializer' an approvable approach?

Idiomatic ruby likes shallow but narrow classes, and plenty of methods, so I think you're right that it's not; however, calling methods to calculate the outcomes and memoize e.g. in instance variables from the initializer IMO would be perfectly fine.

This comment has been minimized.

@pgaspar

pgaspar Dec 4, 2018

Member

Hey Maud,

I think not, I wouldn't do it myself, but why? If the only argument against it is that it will break if we'd add new functionality, I don't find that very convincing.
If we think it's an approach that shouldn't be approved, or should be approved with food for thought, I'd like to add the arguments here that can help new mentors.

My point is that we probably shouldn't even mention it.

Let's think of a different case: in the Minimal solution for approval we have in these notes replace @scores with $scores. It's totally valid Ruby and will pass all our tests. However, as people experienced with Ruby will notice this is not a Reasonable approach at all - yes it works for this specific case with the current tests but it will lead to problems as soon as you continue expanding the class or using it in slightly different ways, not described in the tests (just as the all in initializer approach does). However, probably most if not all attentive mentors would comment on the fact that $scores is a global variable and how that's a bad practice in certain situations not described in the specs (just as the all in initializer approach is).

Here's how it would break:

a = HighScores.new([1, 2, 3])
b = HighScores.new([4, 5, 6])

p a.scores
# => [4,5,6]
p b.scores
# => [4,5,6]

Do I think we should add a note to these mentor notes saying that global variables are bad? No. Should we add a spec showing how this is a bad practice? Also no - this is just one of a long list of bad practices we would need to test against. I expect the mentors to point this out themselves. And if we added a note and/or a spec we'd have to add it to pretty much all exercises, because this is an issue in pretty much all of them.

The same thing happens for the all in initializer approach - I expect mentors to naturally comment on it, but I don't think we should call it a "reasonable" solution, nor do I think that we should add a note or a test in every exercise where using this approach passes the tests (there are a bunch of them!).

We may be running in circles a bit here. Maybe a comment from @kotp, @iHiD or @Insti could help move this either way? Thanks for your comments too @SleeplessByte 👍

This comment has been minimized.

@F3PiX

F3PiX Dec 5, 2018

Contributor

My point is that we probably shouldn't even mention it. <- Pedro

That makes perfect sense, actually!

Let's go with that. 👍

Show resolved Hide resolved tracks/ruby/exercises/high-scores/mentoring.md
@SleeplessByte

I'm a fan of this change, given that my own PR's actually have the "concepts" and "variations" sections.

That said, I don't like the "only have minimal solutions for simple exercises". Either have it or don't. I think "reasonable solutions" with "accepted variations" is more vague and in this case better than steering towards a minimal solution.

Many mentors are IMO too rigid when it comes to accept non-note-reasonable-solutions and using the "this is right" type of notes only encourages that type of behaviour.

Show resolved Hide resolved tracks/ruby/exercises/high-scores/mentoring.md
```ruby
# version 2+

This comment has been minimized.

@SleeplessByte

SleeplessByte Dec 4, 2018

Contributor

I would suggest removing # version 2+, the version should be bound either by the Gemfile, a .ruby-version or whatever -- I don'think you want to deal with n versions of solutions in the end in the mentor notes, which is what this implies to me.

This comment has been minimized.

@F3PiX

F3PiX Dec 4, 2018

Contributor

Actually, I meant the version of the exercise. But as it's confusing, obviously, and there's a changelog, I'll remove it.

@scores = scores
end
def scores # no need to change getter method to `attr_reader`

This comment has been minimized.

@SleeplessByte

SleeplessByte Dec 4, 2018

Contributor

Should this comment really be in the solution? IMO it's better as a block of text below this with an explanation why it should not need to change to attr_reader. I think in this case you meant something along the lines of "it's fine to use a method instead of attr_reader, but it's not clear to me at all.

scores.sort.reverse.take(3)
end
# composing a string either with string interpolation or with an if.. else.. statement

This comment has been minimized.

@SleeplessByte

SleeplessByte Dec 4, 2018

Contributor

Again I would use variations below this, and not inline this in the "minimal" solution.

### Reasonable approaches
- See the one above
- Defining `@scores`, `@personal_best` and `personal_top` in the constructor.

This comment has been minimized.

@SleeplessByte

SleeplessByte Dec 4, 2018

Contributor

I agree with @pgaspar , even though it's not tested for, a brittle solution is not a good guide for a mentor. If your example here is that "ok but TDD, this is barely enough", then why don't we accept shameless green solutions (solutions that basically have hardcoded solutions to the test cases)? However: I completely agree that solutions should not be rejected if not covered by the instructions or test cases. Note here that IMO the instructions are part of the challenge and should not be disregarded.

Example: A mentor on a different track mentored my leap exercise, suggesting I remove the parentheses, because their minimal solution did not have it in the guide. I had to explain that logically the solution to leap without parentheses is not equivalent.

standard lib (and work with methods chaining) than that they know how to optimize.
### Talking points
- `Array#max` takes an argument; `scores.max(3)` works.

This comment has been minimized.

@SleeplessByte

SleeplessByte Dec 4, 2018

Contributor

I would link this to the max function in the Array docs

This comment has been minimized.

@kotp

kotp Dec 5, 2018

Member

Unless the links are dynamic to the version that is promoted with this language, I would avoid the links. It is important that students know where and how to access the documentation. By this exercise, I would say it should not be something to need to link to. And general references, rather than links, will reduce the areas that we need to update because of version changes.

This comment has been minimized.

@SleeplessByte

SleeplessByte Dec 5, 2018

Contributor

I agree with your reasoning.

This comment has been minimized.

@SleeplessByte

SleeplessByte Dec 5, 2018

Contributor

FWIW:

https://ruby-doc.org/core/Array.html#method-i-max always links to the latest stable Ruby core docs.

### Talking points
- `Array#max` takes an argument; `scores.max(3)` works.
- Most submissions retrieve the `personal_top` values with the `Array#[]` method.

This comment has been minimized.

@SleeplessByte

SleeplessByte Dec 4, 2018

Contributor

And therefore that's the right approach? I think this talking point is not clear on "why" it's a talking point.

This comment has been minimized.

@kotp

kotp Dec 5, 2018

Member

"most" is quantitative and may change at any point in time. I would encourage why, without mentioning quantity of submissions, which is a moving target.

### Talking points
- `Array#max` takes an argument; `scores.max(3)` works.
- Most submissions retrieve the `personal_top` values with the `Array#[]` method.
`Array#take` wins in readability. And `Array#max()` wins them all.

This comment has been minimized.

@SleeplessByte

SleeplessByte Dec 4, 2018

Contributor

"wins them all" why? I think this talking point is not clear on "why" it's a talking point.

- Consider `String#squeeze` to avoid hard-to-spot spacing in the string itself.
### Mentor Research
- `String#<< / String.concat` vs. `String#+`?

This comment has been minimized.

@SleeplessByte

SleeplessByte Dec 4, 2018

Contributor

These should be links to the docs. And these notes are pretty meaningless if they don't include what the resolution of the research was.

This comment has been minimized.

@kotp

kotp Dec 5, 2018

Member

I don't agree about the links to the documentation. For example, I know that I can do ri 'String#+' to get to the documentation, and a link is not going to change this. Others may not. Plus the links may decay over time, due to our supported language, or their dynamic nature depending on where the links direct. I am also aware of students using different versions of Ruby, and so they would have to adjust for their own use anyway.

This comment has been minimized.

@SleeplessByte

SleeplessByte Dec 5, 2018

Contributor

When I do ri String#+ it gives me

$ ri String#+
Nothing known about String

String#+ however works, regardless the version. In general ruby doesn't remove methods, ever, so usually links to older versions are not a problem.

Might just be me though :)

This comment has been minimized.

@kotp

kotp Dec 5, 2018

Member

You probably haven't generated the ri documentation for core. But your gems will show. if you set the .gemrc file specifies to do so. Once you generate the core documentation, it will show, if the documentation for gems are showing.

This comment has been minimized.

@SleeplessByte

SleeplessByte Dec 5, 2018

Contributor

Oh sorry. Yes I know that, you know that, but do they ;) ?

Ruby 2.3 will freeze string literals by default.
### Changelog
- Version 2 changed method names "highest" -> "personal_best", "top" -> "personal_top"

This comment has been minimized.

@SleeplessByte

SleeplessByte Dec 4, 2018

Contributor

This is GREAT. I would love if we had this everywhere.

This comment has been minimized.

@sshine

sshine Dec 11, 2018

Contributor

This is very useful indeed!

Especially for breaking changes.

def report
difference = "#{personal_best - latest} short of" unless personal_best == latest
"Your latest score was #{latest}. That's #{difference} your personal best!".squeeze

This comment has been minimized.

@kotp

kotp Dec 5, 2018

Member

This reads strangely. But the point is that there will be an extra space introduced if it is your personal best.

This comment has been minimized.

@kotp

kotp Dec 5, 2018

Member

Never mind, using squeeze corrects for that. 😊

This comment has been minimized.

@SleeplessByte

SleeplessByte Dec 5, 2018

Contributor

Should use squeeze(' '), because "squeeze".squeeze returns squeze.

https://launchschool.com/books/oo_ruby/read/classes_and_objects_part1#accessormethods.
It's recommended to link to this or your favorite explanation, instead of typing the solution for them.
- `sort.reverse` is not very performant, but at this point in the track it's more important that students explore the
standard lib (and work with methods chaining) than that they know how to optimize.

This comment has been minimized.

@kotp

kotp Dec 5, 2018

Member

"standard lib" should be "core" and "library". No need to abbreviate a word, and especially not one that comes up with definitions that may not be clear in the English dictionary. Since this is Ruby, there is a difference between "core" and "standard" and often nothing more than core is necessary. Though I agree it is important that students explore both collections.

This comment has been minimized.

@F3PiX

F3PiX Dec 11, 2018

Contributor

I use 'Core' and 'Standard' when referring to specific Ruby libraries. With lower case 'standard' I meant it in a more general way.

@rpottsoh

This comment has been minimized.

Member

rpottsoh commented Dec 10, 2018

Is there anything keeping this from being merged?

@kotp

This comment has been minimized.

Member

kotp commented Dec 10, 2018

Strictly speaking not as far as I can see. Personally would prefer not to abbreviate words because of the multiple reasons that abbreviations end up being a little harder for non-native speakers. The words being used in that way are likely known from a "I am programmer" land though.

A little bit stronger objection would be "standard library" vs "core library" since for Ruby there is a difference, and core is more the focus than the standard library. (see this review comment).

@F3PiX F3PiX dismissed stale reviews from pgaspar and iHiD via c51d90c Dec 11, 2018

@F3PiX

This comment has been minimized.

Contributor

F3PiX commented Dec 11, 2018

I mainly fixed the errors. There's also some matters of personal taste; feel free to open a new PR to discuss details.

@rpottsoh Sorry for the delay, this is ready to merge now.

@rpottsoh

This comment has been minimized.

Member

rpottsoh commented Dec 11, 2018

Thanks @F3PiX, there is no rush. It just was not clear to me what the hold up was since there were two approvals. I'm happy to merge this if you like, but I tend not to merge if the issuer has write access (or an excessive amount of time has transpired and the PR appears to be OK to merge).

@F3PiX

This comment has been minimized.

Contributor

F3PiX commented Dec 11, 2018

Could you approve it @rpottsoh ? Then I can merge. It says Review required - probably for the last commit.

@rpottsoh

This comment has been minimized.

Member

rpottsoh commented Dec 11, 2018

You got it! 😄

@F3PiX F3PiX merged commit ce54d2d into master Dec 11, 2018

2 checks passed

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

@F3PiX F3PiX deleted the ruby-highscores branch Dec 11, 2018

@rpottsoh

This comment has been minimized.

Member

rpottsoh commented Dec 11, 2018

probably for the last commit.

That commit appears to have dismissed the two approvals as stale.

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