Skip to content

Conversation

@davidmyersdev
Copy link

No description provided.

# index is entirely based on the number of misses
def get_state index
@states[index]
end
Copy link

Choose a reason for hiding this comment

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

The general practice on our team has been to avoid comments when possible. Here for instance, renaming the variable to from index to number_of_misses would do the trick.


def get_word
@word
end
Copy link

Choose a reason for hiding this comment

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

check out attr_reader (and attr_accessor while you're at it) in the ruby documentation.

Copy link
Author

Choose a reason for hiding this comment

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

Once, I finish up the challenges, I'll be going back through to refactor everything. I've been learning quite a bit by going straight through these, and that is one of the things I've picked up on (thanks to RuboCop).

end

def get_lines guesses
@word.chars.map{ |char|
Copy link

Choose a reason for hiding this comment

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

Once you've got the reader method in place, then @word can become just word here.

end

def input_is_valid? input
return !!(input =~ /[a-z]/i) && (input.length == 1)
Copy link

Choose a reason for hiding this comment

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

standard ruby style is to avoid using the return keyword unless it's an early return

Copy link
Author

Choose a reason for hiding this comment

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

Another thing I've realized with further challenges. Will fix soon. Thanks.

Copy link

Choose a reason for hiding this comment

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

gotcha, cool

@guesses = []
@misses = []
@words = WordFactory.get_words
@screen = Screen.new
Copy link

Choose a reason for hiding this comment

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

as mentioned below, some attr_accessors would help here.

Copy link
Author

Choose a reason for hiding this comment

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

Most of these are meant to be private, so is there a reason to assign accessors?

Copy link

Choose a reason for hiding this comment

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

Ok, I see what you're thinking. In the code I've seen here, I'd say that most ruby-folks would use attr_readers or attr_accessors whenever they're referring to instance variables, but maybe there is not much reason to in cases like this. I mean, there is always the aspect of changing everything to a custom getter/setter easily, but that honestly doesn't happen very often.

Copy link
Member

Choose a reason for hiding this comment

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

You can define attr_readers and attr_accessors under a call to private. I recommend always using the methods. Ruby has this stupid behavior: @ivar_with_typo #=> nil. Method calls with typos will raise an exception.

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.

3 participants