-
Notifications
You must be signed in to change notification settings - Fork 21
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
Initial cut #15
base: master
Are you sure you want to change the base?
Initial cut #15
Conversation
def initialize | ||
@people = { | ||
thieves: 5, | ||
officers: 1 } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be simpler if these weren't stuck together in @people
. Try defining them as @thieves
and @officers
.
Good point. |
end | ||
|
||
def successful_crime_rate | ||
if @thieves <= 0 || @officers > thieves |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Whenever these if
conditions get even a little complicated, I think they should be extracted to a private method. What do you think a good name for this condition might be? no_crime_can_occur?
, plenty_of_cops?
, something like that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I prefer no_crime_can_occur? because it captures the meaning of the method without exposing how the method makes that decision.
end | ||
|
||
def no_crimes_can_occur? | ||
@thieves <= 0 || @officers > @thieves |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it is a little better to use the reader methods for instance variables even inside the class. In the off chance they get refactored into real methods someday, this line shouldn't need to change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a way to do that without angering rubocop?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's it complaining about?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was complaining about me not understanding the intent until just now. I think that I have it now.
This is looking real nice. 👍 |
Thank you. |
Thanks @jaybobo @mikegee