-
-
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
OOP exercise #374
OOP exercise #374
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, |
Self-check completed. |
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. А. Чому так? Б. Що робити, якщо в піарі нема оновлень, оскільки не зрозуміло, що треба зробити? В. А якщо я все зробив(ла) і це ментор не рев'юває мої зміни?
Г. Хіба недостатньо того, що я додав(ла) коміт із змінами? Традиційна пропозиція: задай питання по вищенаписаному в студентському чаті. |
Please review. Self-check completed. |
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.
@Yanyshpolska a really good job!
A few improvements/optimizations and we are done.
constructor(...args) { | ||
super(...args); | ||
} |
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.
...args
is a bad practice because IDE intellisense won't be able to hint what exactly is expected here.
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.
Got it.
Fixed.
super.getInfo() + | ||
", " + | ||
this.hands + | ||
", " + | ||
this.legs + | ||
", " + | ||
this.constructor.name |
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.
Benefit from Array#join
.
Please explain benefits.
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 tried the solution with the Array.join and it worked for me. I found a more effortless and more elegant solution than concatenating strings manually. However in the Array.join I can't control the order of parameters so I need to arrange them correctly in the class itself.
For this particular case, it works fine, so I switched to Array.join.
Thanks for the suggestion.
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 didn't look into fixes, but this part "I can't control the order of parameters so I need to arrange them correctly in the class itself." sounds confusing.
What stops you from arranging any items before joining them?
E.g. [ prop9, prop1, prop3].join("")
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.
Took a peek into the changes. Object
methods are still a no go. The reasons behind remain the same since pre-OOP TJSW app.
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.
My bad. Totally forgot than Object methods are not recommended to be used.
super(...args); | ||
} | ||
getInfo() { | ||
return super.getInfo() + ", " + this.constructor.name; |
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 wonder if we can generalize use of this.constructor.name
? Too much repetition across the entire codebase.
Did you try to use it in the base class?
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.
Reworked as you suggested.
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.
@Yanyshpolska great job done!
Look also into objects as functions' arguments. Along with destructuring and default values for skipped properties it gives both readability and great flexibility.
function doSomething({ prop3, prop2 = "default value", prop1}) {
...
}
const meaningfulName = {
prop3: 'a',
prop2: '2',
prop1: 1,
};
const meaningfulName2 = {
prop3: 'a',
prop1: 1,
};
doSomething(meaningfulName);
doSomething(meaningfulName2);
OOP exercise
Demo |
Code base
The code is submitted in a dedicated feature branch.
Only code files are submitted.
Please, review.