-
-
Notifications
You must be signed in to change notification settings - Fork 184
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
Tiny js world #375
Tiny js world #375
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 listed/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. 😒 A Tiny JS World -- (pre-OOP) exercise check listRelates to Object-Oriented JavaScript task. Check-list - definition of done
Universal recommendations:
Also take a note of the requirements above and follow them in all your future projects. By the way, you may proceed to the next task before this one is reviewed and merged. Sincerely yours, |
i have completed self-checks |
@VladimirRutskiy |
@OleksiyRudenko It's code for "10. OOP exercise - practice from Advanced Topics list " task |
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 both basic and advanced 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, |
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! I don't see any code that should create instances of your classes or print their info. It seems that this file is incomplete. Can you fix this, please?
@artem-trubin I'm so sorry. I didn't notice that the file was partly fill |
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, nice work. Let's make it better. See the comments below.
} | ||
|
||
class Human extends Inhabitant{ | ||
constructor (species, name, gender, 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.
Are you sure a Human
needs a species
here? A human can only be human, isn't it?
} | ||
class Woman extends Human{ | ||
constructor(name, gender, saying){ | ||
super('woman', name, gender, 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.
A woman can only be female, so there's no need to ask for a gender in the constructor here.
@artem-trubin Thank you. I fixed all remarks |
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.
Good job!
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.
@VladimirRutskiy very well done!
Although need to improve it for the sake of OOP compliance, which in its turn will make code maintainable and extensible, concerns will be separated across classes.
constructor (name, gender, saying){ | ||
super('human', name, gender, 2, saying); | ||
this.hands = 2; | ||
this.props = [...this.props, 'hands'] |
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.
It is generally a bad practice to directly change properties that are owned by the base class.
Every class should handle its own properties i.e. encapsulate both properties and behaviour.
The same refers to Inhabitant#showProps
.
Let every class handle properties it owns.
Use method overloading to provide seamless user experience.
You will benefit from explicit call to inherited version of the methos via super
.
Task and original project materials offer links to the articles that you will find helpful.
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 have fixed
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.
@VladimirRutskiy make a step back.
legs
were right where they belonged.
My comment referred to this.prop
direct assignment in a child class.
Actually, you do not need this.props
at all. It is brought from pre-OOP version obviously, and there it was a workaround. OOP paradigm helps to avoid that sort of workarounds.
Let's restore previous version. I will explain some theory on that and we will rather merge that previous version. It was better.
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.
Good job!
OOP exercise
Demo |
Code base
The code is submitted in a dedicated feature branch.
Only code files are submitted.
Please, review.