-
-
Notifications
You must be signed in to change notification settings - Fork 185
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
OOP exercise #93
OOP exercise #93
Conversation
Thank you for contributing to this repo! ❤️️ A Tiny JS World -- OOP exercise check listRelates to Let's do some self-checks to fix most common issues and to make some improvements to the code while reviewers find some time to dedicate it to your submission. Go through the checklist below, fix code as appropriate and mark fulfilled requirements when you genuinely believe you are done. Please, feel free to ask questions here or seek for help in the Students' chat. Check-list - definition of doneMinimal requirements to meet:
Optional level up (not required to implement):
Bonus:
Helpful resources: Sincerely yours, |
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.
Hey! Let's improve your work, check comments below.
} | ||
} | ||
|
||
/* Human */ |
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.
Is there really sense in these comments? Human class is called Human
already. Same for others.
@@ -0,0 +1,96 @@ | |||
/* Main Class */ | |||
class Mammal { |
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.
Mammal
class should store values that are exclusive to it. Does every mammal have hands or wings?
} | ||
|
||
aboutMe() { | ||
return `${this.saying} My name is ${this.name}. I'm ${this.species} ${this.gender}. I have ${this.legs} legs.`; |
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.
Instead of rewriting the method, you can use the Mammal
's aboutMe
method in conjunction with a new one. You can call it from super
. You just need to change it a little so that properties don't repeat.
} | ||
} | ||
|
||
let john = new Man('John', 'Hello'); |
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.
These are consts.
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.
Hi! It is an improvement for sure, but there are still problems. Please read the comment below.
constructor (name, saying = 'Hi') { | ||
super(name, saying); | ||
this.gender = 'female'; | ||
this.saying = saying; |
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 are a lot of repeating actions, for example - saying
. When you create Woman
, you pass a string as a saying
to it. It gets it as a constructor parameter and then passes it to the super
constructor. Later, on the 41
line it assigns this value to the this. saying
property. But what does super
do with it? Well, here, basically there's broken logic since the second parameter of the Human
constructor is gender. So you pass saying as gender. And you also rewrite gender
on line 40. Do you see what I'm saying? Please restructure your code, maybe even from scratch to make sure that every class is responsible for its exclusive properties and if something needs to be set from outside (like names or genders) it is passed properly.
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.
Something went wrong since the last review. Where did the Inhabitant
class go? It looks like you redid your structure, but there are some flaws now. Please rewrite it again, considering these points:
- Your whole hierarchy should have one common ancestor.
- If you see a property present among all descendants of some ancestor - it should be lifted up the hierarchy tree and assigned to the ancestor.
- Do not rewrite the method for getting information entirely. If your ancestor tells about some property and you want to add something to it - add it, don't rewrite the whole message.
That's it, well done! |
* js core code * oop exercise with classes * deleted frogger app.js file * fixed func name, renamed let to const, rewrite classes * rewrite classes * rewrite classes * some errors with upstream * add inhabitant class Co-authored-by: asa <pandemix331@gmail.com>
OOP exercise
Demo |
Code
The code is submitted in a dedicated feature branch.
Only code files are submitted.
Please, review.