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

Clean up of code to match rubocop rules. #4

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Conversation

gwadej
Copy link

@gwadej gwadej commented Oct 5, 2015

Not sure how terse to go with some of the code. I'm still not 100% with
accepted Ruby idioms.

@chivygab @jaybobo Please review.

G. Wade Johnson added 2 commits October 5, 2015 13:25
Not sure how terse to go with some of the code. I'm still not 100% with
accepted Ruby idioms.
@gwadej
Copy link
Author

gwadej commented Oct 6, 2015

As suggested by Jay: @mikegee please review.

def initialize
@people = {
thieves: 5,
Officers: 1
Copy link
Member

Choose a reason for hiding this comment

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

Is this hash internal such that downcasing Officers to officers won't break anything?

Copy link
Member

Choose a reason for hiding this comment

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

Actually, what about ditching the hash for two instance variables (@thieves & @officers)?

Copy link
Author

Choose a reason for hiding this comment

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

I didn't want to make changes that were not based on rubocop output. But, both of these changes would definitely improve the code.

@mikegee
Copy link
Member

mikegee commented Oct 6, 2015

This is very readable now. 👍

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

Successfully merging this pull request may close these issues.

2 participants