-
-
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
a-tiny-js-world #390
a-tiny-js-world #390
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, |
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.
Nice work! Let's improve it. Check the comments below.
saying: cat["saying"], | ||
}; | ||
dog.friend = `${woman.name},${man.name},${cat.name}`; | ||
cat.friend = undefined; |
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.
undefined
is a default value for some variable that was not given a value or for places where we wait for a value, but don't get it as a result of some mistake or something like that. If you want to specify something as "nothing" or "empty" use null
. But of course, the end user of the app/website should see neither null
nor undefined
. You release your undefined
to the eyes of the user, and that is a mistake. For the sake of this task, animals are allowed to have 0
hands, for example. And if someone doesn't have friends
- you should just ignore that property.
hands: 2, | ||
saying: cat["saying"], | ||
}; | ||
dog.friend = `${woman.name},${man.name},${cat.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.
friend
should be plural, I guess. And it should be an array since here is where you keep your "raw" data, a model if you will. Then some other code should pick this data and format it to display.
gender: "female", | ||
legs: 2, | ||
hands: 2, | ||
saying: cat["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.
Catwoman's saying should change if we change the cat's saying any time. Does yours?
const inhabitants = [dog, cat, woman, man, cat_woman]; | ||
inhabitants.map((item) => { | ||
let res = ""; | ||
for (let key in item) { |
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.
You should use Array.forEach
and other similar methods everywhere where it's possible. It increases the readability of the code a lot. Also, don't rely on cycling through keys in the objects, you never know for sure which keys are present or in which order. Find a way to specify the keys by hand and then get values from objects.
*/ | ||
const inhabitants = [dog, cat, woman, man, cat_woman]; | ||
inhabitants.map((item) => { | ||
let res = ""; |
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 stacking values into the string like that, it's cleaner to prepare some values and then .join()
them.
gender: "female", | ||
legs: 2, | ||
hands: 2, | ||
saying: cat.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.
Nice work! Your catwoman's saying is still not connected to cat's saying. Yes, catwoman says what cat said initially, but if we change cat's saying later in code, catwoman will not change her saying accordingly.
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.
Yes, this is one way of doing so. I'm going to approve that, but be aware that following this, the Object.setPrototypeOf
is a very slow operation and you could use Object.create
to achieve the same goal. You could've also just used bind
. That's just for the future, look into those methods. Great work!
A-Tiny-JS-World
Demo |
Code base
The code is submitted in a dedicated feature branch.
Only code files are submitted.
Please, review.