Closed guygastineau closed 6 years ago
If this type of hack is idiomatic for Ruby then please feel free to tell me so.
Like I said above - it just seems like a hack to me.
Clock description Clock tests (Ruby) Clock canonical data
Hi @guygastineau, thanks for raising this issue. It's something that I've also been thinking about recently too.
As an experienced Ruby dev using a class method as an alternate constructor is not uncommon. Have a think about this: What is the difference between Clock.new
and Clock.at
?
That said I'm not a fan of positional arguments, and would rewrite it to use keyword arguments: Instead of Clock.at(23,59)
I'd do Clock.new(hour: 23, minute: 59)
I also think the tests should be changed, I didn't like that minute addition was done with a Clock instance + a Fixnum, and would rather see it be Clock + Clock
Rather than:
def test_add_minutes
assert_equal "10:03", (Clock.at(10, 0) + 3).to_s
end
We'd have:
def test_add_minutes
assert_equal '10:03', (Clock.at(10, 0) + Clock.new(minutes: 3)).to_s
end
The exercise description is pretty sparse, so I think we have some scope for rewriting the tests how we like.
I think it would be a good idea to rewrite the clock tests.
I'm happy to help you if you want to take a shot at this. What do you think?
P.S. - It would also be good (but not 100% necessary) to write a generator for the tests while we're at it.
@Insti thank you.
Yeah, I might have some time to work on this this week.
I'm also busy getting the bash tests.sh up to date with the generator I built for it. It is simpler in structure than the generator framework for Ruby and the templates are just ERB files
That being said. I would definitely like to make a generator for this test.
A student told me recently that the Clock exercise reminds them more of a Timestamp than a clock.
@kotp I can see that.
When I first started this exercise I saw the to_s
's in the test and thought it was the built-in #to_s.
If you all agree I think that should be renamed to be clearer and more descriptive. - also to something without an already reserved namespace.
I would definitely like to make a generator for this test.
Great! :heart: Let me know if you have any questions along the way. Feel free to make a WIP (Work In Progress) Pull Request once you have some code to discuss.
Clock.at(12, 12).to_s
using Clock.at(12, 12).format
? # Of course the arguments could still be keyword arguments instead of positional.(Clock.at(10, 0) + 3).to_s
using
(Clock.at(hour: 10, minute: 0) + Clock.at(minute: 3).minutes).format
attr_reader
to access the total minutes from another clock object which could allow for future tests in which the added Clock
object also has been given hours, eg. (Clock.at(hour: 10, minute: 0) + Clock.at(hour: 12, minute: 43).minutes).format
def +(minutes)
would still function the same as it does now, but there would be some extra functionality overall from the other changes. Some extra clarity too I think.Sounds nice!
One more consideration in favor of keeping to_s
, from a curriculum overview point of view: AFAIK the Clock exercise is the first exercise in the track that explicitly requires to implement a custom to_s
. And I think that is very useful to get experience with. (I always forget about those kind of things.)
In general I like where this is going, but I don't know the Clock exercise very well.
@F3PiX
Thank you for your input.
So, it sounds like you are saying that it is idiomatic to implement a custom to_s
method inside a class for Ruby?
I am familiar with duck punching, although it seems discouraged. I recognized that this is not duck punching.
I suppose to me it seems more sane not to mess with the namespace reserved for to_s
, but if doing so would be idiomatic then I really don't have the experience to argue about it.
I will submit a WIP PR soon. I would like to get a few things more or less decide first:
Clock.new
will replace Clock.at
to_s
will be kept(Clock.new(hour: 12, minute: 12) + Clock.new(minute: 12)).to_s
Clock.at
use Clock.new(hour: 1, minute: 2)
- Instead of Clock.at(12, 12).to_s using Clock.at(12, 12).format ? # Of course the arguments could still be keyword arguments instead of positional.
Definitely use .to_s
. It is idiomatic ruby.
- Instead of (Clock.at(10, 0) + 3).to_s using (Clock.at(hour: 10, minute: 0) + Clock.at(minute: 3).minutes).format
Lord no!
Add Clock
objects directly.
(Clock.new(hour: 10, minute: 0) + Clock.new(minute: 3)).to_s
I suppose this way kind of suggests the students implement attr_reader to access the total minutes from another clock object which could allow for future tests ....
This is implementation detail that we should not be testing. There are several ways to implement a solution, some of which won't do this. The student is free to write their own intermediate tests if they want.
@Insti
Thank you for the feedback. I guess I was confused about how the other object would be added to first one.
Since it is passed as a parameter to the first Clock obj I guess the +
method can access it's methods to retrieve the information however the student wants.
Thanks for getting me to think about that. I think I have it figured out.
UPDATE: I edited the checklist above. Let me know if that looks good now.
Hi all,
While doing the clock exercise I noticed:
(Clock.at(12, 12) + 12).to_s
the solution requires making an instance of the class.Clock.new
insidedef self.at
.Thank you