JS OOP Exercise task #454
JS OOP Exercise task #454
Conversation
|
Hey! Congratulations on your PR! 😎😎😎 Let's do some self-checks to fix most common issues and to make some improvements to the code before reviewers put their hands on the code. Go through the requirements/most common mistakes linked below and fix the code as appropriate. If you have any questions to requirements/common mistakes feel free asking them here or in Students' chat. When you genuinely believe you are done put a comment stating that you have completed self-checks and fixed code accordingly. Also, be aware, that if you would silently ignore this recommendation, a mentor can think that you are still working on fixes. And your PR will not be reviewed. 😒 Please, make sure that your code follows the requirements based on the most common mistakes as well as requirements from the original task. Universal recommendations:
By the way, you may proceed to the next task before this one is reviewed and merged. Sincerely yours, |
|
Self-checks done. |
OleksiyRudenko
added
the
self-check-done
There was a problem hiding this comment.
@franchukv handling properties looks quite inconsistent.
Please review your code and improve it.
| super(species, gender, name, status, saying, friends); | ||
| this.hands = 2; | ||
| this.feets = 2; | ||
| this.properties = [...this.properties, 'hands', 'feets', 'saying']; |
There was a problem hiding this comment.
A subclass shouldn't handle any properties owned by a super class.
| man.objToString(), | ||
| woman.objToString(woman.toMakeFriends()), | ||
| dog.objToString(dog.toMakeFriends()), | ||
| cat.objToString(cat.toMakeFriends()), | ||
| catwoman.objToString(catwoman.toMakeFriends()), |
There was a problem hiding this comment.
What construction object.objToString(obj.toMakeFriends()) does?
There was a problem hiding this comment.
This construction was needed to save the order of properties when we print them, I rewrote code, please review new version
| super('human', gender, name, status, saying, friends); | ||
| this.legs = 2; | ||
| this.hands = 2; | ||
| this.properties = [...this.properties, 'hands', 'legs']; |
There was a problem hiding this comment.
You should avoid handling base class' props directly.
| } | ||
|
|
||
| inhabinantPropetriesToString() { | ||
| return this.properties.map((property) => this[property]).join('; '); |
There was a problem hiding this comment.
You should let child classes handle their own properties end-to-end.
Why that? Encapsulation.
Why encapsulation is important? Otherwise any change to any class properties forces us to change code in other classes. This leads to errors.
JS OOP Exercise
Demo | Code base
The code is submitted in a dedicated feature branch.
Only code files are submitted.
Please, review.