exercism / website-copy

A repository for exercism's website's copy
203 stars 950 forks source link

Ruby/Luhn Discuss Luhn notes #876

Open emcoding opened 5 years ago

emcoding commented 5 years ago

For discussion, improvements and filling the gaps in #875

iHiD commented 5 years ago

Thanks for this. I've opened a PR for what I consider to be an essential change.

I think it would be nice to have an "optimal" solution in the mentor notes here too, so we still have something to teach the people that get the minimal solution first time.

A few other suggestions:

emcoding commented 5 years ago

Thanks for the PR and catching that! 🏅

I think initialize be private too.

initialize is private by design, and it looks like by convention, it is positioned in the public interface, without actually being public. (What I found: Normally, a subclass can override its parent's visibility, because Ruby adds a call to super that makes the change possible. 🙃 My guess is that that magic has been taken out for initialize.)

luhn_sum % 10 == 0 - I like .zero?

I felt it's too much depending on personal taste to add it to the notes. It's always debatable what should and shouldn't be in the notes, so this was just my judgment call. And if there wasn't much else to talk about in the notes, I probably would have mentioned it. Also, it should have come up in earlier exercises, and we can't repeat everything that has been handled before. But still: just a judgment call.

I think it would be nice to have an "optimal" solution in the mentor notes here too

Let's discuss that optimal solution first. Then worry about how to squeeze it into the Notes.

tenebrousedge commented 5 years ago

I think that the approach given in the notes is somewhat flawed.

require 'scanf'
PAIRS = (0..9).zip([0, 2, 4, 6, 8, 1, 3, 5, 7, 9])
def luhn_sum(string)
  string.delete(' ').reverse.scanf("%1d%1d") do | even, odd = 0 |
    even + PAIRS[odd]
  end.sum
end

It's not necessary to perform multiplication in the loop, and scanf allows us to chunk and typecast the string in one go.