-
-
Notifications
You must be signed in to change notification settings - Fork 183
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 #443
A tiny JS world #443
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, |
@unabyband please double check what the bot asked you to do. |
Double check is done! |
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.
@unabyband good job!
Let's polish it.
// var test = man; | ||
// test.saying = "<h2>Where's the money, Lebowski?</h2>"; | ||
|
||
function printInhabitants(inhabitant) { |
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.
Does it print inhabitantS?
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.
Oh, sure! Thank you :)
inhabitant.species, | ||
"<strong>" + inhabitant.name + "</strong>", | ||
inhabitant.gender, | ||
inhabitant.legs, | ||
inhabitant.hands, | ||
"<em>" + inhabitant.saying + "</em>", | ||
inhabitant.friends, |
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.
There should be a way to avoid repetition of inhabitant.
.
How about deconstruction in function parameters definition?
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've been thinking about this way past several days... I definitely want to avoid repetition, but still don't realize how to print different values with different html-styles without repetition....
Okay, I'll try to make it again
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 was referrring specifically repetition of inhabitant.
fragment.
Check object deconstruction and how you could use it at function parameters definition.
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've fixed it, take a look again, please.
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.
Now it is repetition of args.
:)
Check destructuring. And here is exactly your case.
Disregard. I was looking into some other code.
Check list is still valid. Go through it. Then make a step back and look into destructuring as commented here |
I should avoid using "for...in" cycle here, shouldn't I? |
Yes, you are. And there is an explanation as to why, and the reason is valid. Now, take a step back in your code version and make another try |
I'm not completely sure if it's exactly what I had to do, but please review again. |
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.
@unabyband DRY code still can be customized.
Just needs a bit of creativity.
const res = [species, name, gender, legs, hands, saying, friends]; | ||
print (res.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.
Why do you need res
(apart of the fact this name violates Universal requirements)?
Also bring back original formatting for name and 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.
Done! Please re-review again
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.
@unabyband great job!
As you have an empty line at the end of file, this ^^ means you have non-Unix newline (most likely you have Windows CRLF).
Check your IDE and git config to ensure newline is Unix-style (LF).
Why this is important? When co-developers have different newline settings this may cause code conflicts and/or mark changes in the files where there are no actually changes beside difference in newline characters.
Thanks a lot! I found it in VSC and switch "EOL" to LF setting. |
A tiny JS world
Demo |
Code base
The code is submitted in a dedicated feature branch.
Only code files are submitted.
Please, review.