Skip to content

Conversation

@valeria-martinez
Copy link

No description provided.

response += "... You're welcome."
response
end

Copy link

@ssachid ssachid Nov 29, 2016

Choose a reason for hiding this comment

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

teach_stuff method is common in both the SeniorTeacher and ApprenticeTeacher classes except for a few words. Can you think of a way to extract it into the Parent class?

"High five!"
end

end No newline at end of file
Copy link

Choose a reason for hiding this comment

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

👍


class Student < People
include HighFiveable

Copy link

Choose a reason for hiding this comment

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

Class names should be singular. Class name should have been "Person" here.

class Student  < Person
end


class TeachingPosition < People

attr_accessor :num, :target_raise, :salary, :number
Copy link

@ssachid ssachid Nov 29, 2016

Choose a reason for hiding this comment

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

This reads as "a Teaching position is a kind of person" which doesn't sound quite right. A good alternative would be to call the class name "Teacher" instead of "TeachingPosition"

class ApprenticeTeacher < Teacher   -> Apprentice teacher is a Teacher
end

class SeniorTeacher < Teacher  -> Senior teacher is a Teacher
end

class Teacher < Person -> Teacher is a Person
end

class Student < Person   -> Student is a Person
end

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