sum-of-multiples: added generator#658
Conversation
|
|
||
| def test_multiples_of_43_or_47_up_to_10000 | ||
| skip | ||
| assert_equal 2203160, SumOfMultiples.new(43, 47).to(10000) |
There was a problem hiding this comment.
It would be good to use number underscoring with big numbers.
| class SumOfMultiplesCase < Generator::ExerciseCase | ||
| def workload | ||
| indent_lines(["assert_equal #{expected}, SumOfMultiples.new" \ | ||
| "(#{factors.join(', ')}).to(#{limit})"] , 4) |
There was a problem hiding this comment.
- I like to try really hard to avoid line continuations like this.
- Did you know
ExerciseCasehas anassert_equalhelper method?
There was a problem hiding this comment.
I took a look at the assert_equal helper method, but it's calling inspect on the underscored value (which is a string), and then interpolating that into the assertion. This results in the assertions looking like: assert_equal "1_000", ..., when what we want is to assert two numbers.
There was a problem hiding this comment.
Hmm interesting. Thanks.
The whole assertion helper thing is quite new and has many unexplored edge cases.
Creating an issue about this might be a good idea.
| class SumOfMultiplesTest < Minitest::Test | ||
| def test_multiples_of_3_or_5_up_to_1 | ||
| # skip | ||
| assert_equal 0, SumOfMultiples.new(3, 5).to(1) |
There was a problem hiding this comment.
Probably unrelated to generator, but would it be interesting to allow SumOfMultiples to return something more Enumerable?
SumOfMultiples.new(3,5).take(1)
cc: @kotp
There was a problem hiding this comment.
I'm really liking the idea of changing the method name from to to take
|
Nice 👍 |
ajwann
force-pushed
the
add-generator-for-sum-of-multiples
branch
from
b6f9d57 to
6b972a2
Compare
|
@Insti I was able to add underscoring at the cost of using the |
|
Thanks @ajwann The other changes look good.
This would be super 👍 |
kotp
left a comment
kotp
left a comment
There was a problem hiding this comment.
Looks good, glad this is going to be generated, but not positive about the method name change.
| end | ||
|
|
||
| def to(limit) | ||
| def take(limit) |
There was a problem hiding this comment.
What inspired the method name change?
Take a limit is a different idea potentially than to a limit.
The "to" implies an exclusive limit, while take suggests an inclusive limit. An example of this in English is "go to the wall" which implies do not go all the way into the wall, or on the wall, just approach it as close as you can. The Readme is clear that we are to exclude the limit.
There was a problem hiding this comment.
Although I had forgotten that the limit was excluded, so the name change may be inappropriate.
There was a problem hiding this comment.
take is also not good since we're not actually taking an array, we just want the sum.
I think I've convinced myself that the method name change was a bad idea.
There was a problem hiding this comment.
Haha sounds good. I'll add the line break and take out the new method name. 😂
ajwann
force-pushed
the
add-generator-for-sum-of-multiples
branch
from
6b972a2 to
3568975
Compare
| def workload | ||
| assert_expected = "assert_equal #{expected.underscore}" | ||
| value = "SumOfMultiples.new(#{factors.join(', ')}).to(#{limit})" | ||
| indent_lines(["#{assert_expected}, #{value}"], 4) |
There was a problem hiding this comment.
Feedback rather than a change request:
The grouping here is logically confusing as in assert_expected you have only 2/3 of a valid assert_equal call.
value is good.
constructing the string at the end is ok due to your variable naming, but confusing because assert_expected looks like a minitest method but it isn't one.
I would have done this:
result = "SumOfMultiples.new(#{factors.join(', ')}).to(#{limit})"
indent_lines(["assert_equal #{expected.underscore}, #{result}"], 4)
|
Bringing it in, other than the mentioned code by @Insti it looks good to me. We can change that up if it ends up being an irritant. I can see it being that way, on the next round. I can also see the |
3 participants
references #396