-
Notifications
You must be signed in to change notification settings - Fork 74
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
Finish challenge #68
base: master
Are you sure you want to change the base?
Finish challenge #68
Conversation
end | ||
|
||
def dead? | ||
!self.alive |
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.
You are consistently using the self.attribute
for setting/changing values and just the bare attribute
for reading values, except this one reader. Maybe change this one to !alive
?
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.
@mikegee what's your preference for defining attributes? ivars or using self as done here? And why? :D
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 defining the attribute methods and using them exclusively. Instance variables with typos raise no error, Ruby happily returns nil
. Method calls with typos raise NoMethodError
. I prefer feedback as early as possible.
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.
On this particular line, I suggest removing the self.
prefix but continuing to call the reader method.
Looks great! |
@jaybobo Last one!