Skip to content
This repository has been archived by the owner on Jan 29, 2021. It is now read-only.

Update ruby/clock example solution to match style guide #83

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

isen0011
Copy link

@isen0011 isen0011 commented Oct 2, 2018

The ruby style guide currently suggests to use format or sprintf instead of String#%.

In addition, the self in the hash method is redundant, the style guide also recommends not including it here.

The [ruby style guide](https://github.com/rubocop-hq/ruby-style-guide#sprintf)
currently suggests to use `format` or `sprintf` instead of `String#%`.

In addition, the `self` in the hash method is redundant, the [style guide](https://github.com/rubocop-hq/ruby-style-guide#no-self-unless-required)
also recommends not including it here.
@iHiD
Copy link
Member

iHiD commented Oct 4, 2018

Thanks for the suggestion :) I personally disagree with the rubocop style guide on this one. I think % is a perfectly valid method and more elegant than format. What do other @exercism/ruby-contributors think?

@emcoding
Copy link
Contributor

emcoding commented Oct 5, 2018

I agree with both: that % is valid and more elegant, and that format is more expressive. So' I'd suggest to accept both and add that as a Talking Point to the Mentor Notes.
Because of the expressiveness, I tend towards using format in the example solution. Not feeling strongly about it, though.

@kotp
Copy link
Member

kotp commented Oct 8, 2018

@iHiD I prefer the String#% syntax, though it is concise and magical at first. It grows on you, I think. But it is definitely more familiar in the format or sprintf (especially) forms if you come from some other languages.

@iHiD
Copy link
Member

iHiD commented Oct 9, 2018

@kotp Yes. I agree with you.

@isen0011 What do you think about not changing the code, but adding a discussion point on this instead? I feel that % is more ideomatic (because it's being called on the Ruby object that's in code, rather than being called on Kernel), but that format is easier as a transition from a different language, so it would be nice to have both referenced in the document.

@F3PiX Would you be happy with that?

@kotp
Copy link
Member

kotp commented Oct 9, 2018

because it's being called on the Ruby object that's in code

Specifically being called on a String object, even.

@isen0011
Copy link
Author

isen0011 commented Oct 9, 2018

@iHiD, that makes sense to me. I had thought that format was more widespread in usage - this discussion has been enlightening! I'll submit a new version later tonight for further discussion.

@@ -17,7 +17,7 @@ class Clock
end

def to_s
"%02d:%02d" % time.divmod(MINUTES_PER_HOUR)
format("%02d:%02d", *time.divmod(MINUTES_PER_HOUR))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The style guide is only a guide. I prefer the original line as less noisy. Yes, somewhat mystical the first time you see it. Since we are talking about "Style Guide" I am thinking Rubocop here. It would also suggest not using double quotes, I believe. The single quotes help re-inforce that this is not some trick of "string interpolation" but rather substitution.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I looked at the code when I dropped in, not realizing that I had already responded to the preference. 😊 🏵️ :

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changing to single quotes is an awesome idea! I did think it was a string interpolation trick, the first time I saw it. 👍 The single quotes are really meaningful here.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants