-
Notifications
You must be signed in to change notification settings - Fork 16
Derekpham #22
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
base: master
Are you sure you want to change the base?
Derekpham #22
Conversation
| attr_reader :performance_rating | ||
|
|
||
| def initialize(options={}) | ||
| super(options) |
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.
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.
Because super is used incorrectly here. Refer my comments above.
| include DbcMod | ||
|
|
||
| def initialize(options = {}) | ||
| super(options) |
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.
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.
Refer my comments above.
Refactor you solution and tag me again.
| @age = options.fetch(:age, 0) | ||
| @name = options.fetch(:name, "") | ||
| @target_raise = 800 | ||
| super(options) |
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.
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.
super is a keyword which executes the same method from the parent class.
Since your ApprenticeTeacher class is not inheriting from any parent class, super is not useful here.
| @target_raise = 800 | ||
| super(options) | ||
| @phase = 3 | ||
| end |
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.
In object oriented programming, we tend to prefer using inheritance over modules. This way will certainly work, but we generally only use modules for things that aren't central to the object they live in, or which don't make sense in an existing superclass (and it doesn't make sense to add in a middle layer superclass). In this case, this behavior is very central to what it means to be a teacher at dbc, so they should be part of the superclass for apprentice and senior teacher, not a separate module.
| super(options) | ||
| @phase = 3 | ||
| end | ||
|
|
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.
So in this case, the common state and behavior between the ApprenticeTeacher and SeniorTeacher should be extracted into a parent class named Teacher.
class ApprenticeTeacher < Teacher
end
class SeniorTeacher < Teacher
end| @@ -0,0 +1,31 @@ | |||
| module DbcMod | |||
| attr_reader :age, :phase | |||
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.
This should be a class named Teacher
| @age = options.fetch(:age, 0) | ||
| @name = options.fetch(:name, "") | ||
| end | ||
|
|
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.
Since modules cannot be instantiated, you should not be defining the initialize method inside the module. Further, modules don't hold state. They are used to hold repeated behavior that exists across classes which do not share a common inheritance relationship.
| def offer_high_five | ||
| "High five!" | ||
| end | ||
|
|
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.
This method can go into a separate module called "HighFieveable"
Usually for modules that are going to be included in classes, we use "-able" names, so here it'd be HighFiveable.
| @@ -0,0 +1,32 @@ | |||
| module TeacherMod | |||
| attr_reader :salary, :target_raise | |||
|
|
|||
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.
This should be a class - Teacher
No description provided.