tiny js world oop #378
tiny js world oop #378
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 -- OOP exercise check listRelates to A Tiny JS World OOP exercise. Check-list - definition of doneMinimal requirements to meet:
Optional level up (not required to implement):
Bonus:
Helpful resources: 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, |
|
This issue has been automatically marked as stale because there were no activity during last 14 days. It will be closed in 7 days if no further activity occurs. Thank you for your contributions. А. Чому так? Б. Що робити, якщо в піарі нема оновлень, оскільки не зрозуміло, що треба зробити? В. А якщо я все зробив(ла) і це ментор не рев'юває мої зміни?
Г. Хіба недостатньо того, що я додав(ла) коміт із змінами? Традиційна пропозиція: задай питання по вищенаписаному в студентському чаті. |
|
I went through the self-check list, please, review. |
OleksiyRudenko
added
the
self-check-done
There was a problem hiding this comment.
@bmukha very well done.
A few improvements and it gonna be perfect if not ideal.
| .map((prop) => | ||
| Array.isArray(this[prop]) ? this[prop].join(', ') : this[prop] | ||
| ) | ||
| .filter((value) => Boolean(value)) |
There was a problem hiding this comment.
You may want to filter values before converting them
There was a problem hiding this comment.
I'm a bit confused about this one. In the previous version of this task, I deliberately rewrote that piece this way after your suggestions. And it really simplified my code because I can get rid of all the falsy values with one filter (because I convert possible empty arrays (still truthy values) into empty strings (falsy values) beforehand. Here are the links to our discussion:
Your suggestion
My reply
So, unless I'm missing something, I guess it is better to stick to this realisation. What do you think?
There was a problem hiding this comment.
Oh, I see. Makes sense.
I was thinking of rather null or undefined to denote no friends situation.
Empty array is a valid approach. Keep this fragment as it is.
| } | ||
|
|
||
| props() { | ||
| return ['specie', 'name', 'gender', 'legs', 'hands', 'saying', 'friends'] |
There was a problem hiding this comment.
Every class should handle the properties it owns immediately. hands is not Character's own property.
Method overloading and explicit call to inherited version of the method are to the rescue.
Study what super can do. Original project docs offer 3 relevant articles. There is an answer there.
There was a problem hiding this comment.
I kinda thought that preserving the order of props in the resulting string across all entities is a key requirement of this task. Am I wrong? Because I just can't see the easy way to achieve this goal using super. Should I handle inherited props using super, concatenate own properties to the end of the string and call it a day or I must come up with some kind of solution to put them in the correct places in the string? Another approach I can think of - is to make separate props array for each class. This way I can preserve desired order of props for every class, and I don't need to use the super keyword at all, I can handle everything by calling a slightly modified inherited method from the very first class. Which approach should I take?
There was a problem hiding this comment.
Should I handle inherited props using super, concatenate own properties to the end of the string and call it a day or I must come up with some kind of solution to put them in the correct places in the string?
Let's put aside the solution where we may want insert child's props between parents' props. Let's only append or prepend child's props. For the sake of simplicity
| const selina = new CatWomen('Selina', []); | ||
|
|
||
| [beethoven, grizabella, clyde, bonnie, selina].forEach((character) => | ||
| print(character.props()) |
There was a problem hiding this comment.
A method/function name should start with a verb as they denote actions.
| */ | ||
|
|
||
| class Character { | ||
| constructor(specie, name, gender, saying, friends, legs) { |
There was a problem hiding this comment.
You will benefit from setting default values (here and/or other classes' constructor definitions).
Also consider objects as arguments and object destructuring (which also allows to assign default values to missing props) in function props. This will give you greater flexibility in property management.
|
I made the changes you requested. Please, take a look. |
| specie: this.specie, | ||
| name: this.name, | ||
| gender: this.gender, | ||
| legs: this.legs = 2, | ||
| saying: this.saying, | ||
| friends: this.friends = [], |
There was a problem hiding this comment.
At the moment of instantiation this refers to not what you think it does.
{species} will extract property species from an object passed as a parameter.
In destructuring you only need to list properties you want to extract, providing default values where needed.
There is a pattern for "renaming" when destructuring, but it is not the case here
There was a problem hiding this comment.
@OleksiyRudenko Updated again, please, review.
tiny js world oop
Demo |
Code base
The code is submitted in a dedicated feature branch.
Only code files are submitted.
Please, review.