Skip to content
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

options/options1 comment could lead to incorrect interpretation of the problem #1847

Open
tomasguinzburg opened this issue Feb 3, 2024 · 2 comments

Comments

@tomasguinzburg
Copy link

tomasguinzburg commented Feb 3, 2024

rustlings/excercises/options/options1.rs invites to use the Option types to manage the return value of a function that checks whether there's icecream or not.

Both scenarios where there's a known amount of icecream are expected to be Some values, and the excercise asks to "gracefully handle values where the hour of the day is larger than 23". Sure, using a None is a possibility, but another one is to just time_of_day % 24, specially in the context of the comment:

// We use the 24-hour system here, so 10PM is a value of 22 and 12AM is a
// value of 0 The Option output should gracefully handle cases where
// time_of_day > 23.

Which I think sounds more biased towards the modulo solution than the behaviour expected from the tests. Maybe some kind of rephrasing as to what is understood as "graceful" could be more leading as to why we are using an Option as return type here. Is the function only supposed to work today?

Will the ice-cream be automatically replenished every day at zero hours? I think both of this statements are equally funny and far-fetched, and nothing points clearly towards it being the former and not the latter that is true.

@bangjiehan
Copy link

I have same comment about this exercise.

@shadows-withal
Copy link
Member

Right, I agree that the comment could be rephrased to explain that if time_of_day = 493, that's an invalid state, and that we want to handle that gracefully by returning None. If someone wants to go for a PR, I'll happily merge it, otherwise I'll get to it at some point 👍

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

No branches or pull requests

3 participants