-
-
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 #518
OOP Exercise #518
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 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, |
I've made self-checking, please review |
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 one of the purposes of the module is to learn something new and also demonstrate a practical grip over this new knowledge.
Also techniques learned earlier still worth re-using.
this.species = species; | ||
this.name = name; | ||
this.gender = gender; | ||
this.limbs = limbs; |
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.
Introduce legs and hands and/or paws. In OOP-compliant manner.
Will be happy to see if you mastered OOP - that's the purpose of the module.
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!
all next arguments are indexes of his friends. | ||
I know, it's optional, but hoping my solution is not too primitive*/ | ||
|
||
function addFriends(...arguments) { |
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.
With current signature won't let me know what exactly is expected by this function at call time, without reading the source code.
Code is written for other people.
addFriends(0, 3, 7); | ||
addFriends(1, 4, 5, 6); | ||
addFriends(2, 5, 6, 9); | ||
addFriends(3, 0, 7); | ||
addFriends(4, 1, 5, 6, 8); | ||
addFriends(5, 2, 6, 9); | ||
addFriends(8, 4); | ||
addFriends(9, 1, 2); |
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.
The function signature looked like all arguments represent the same role. This example doesn't really give an idea of how to add another friend pack. Are these numbers a sort of ids?
Make it more obvious
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.
Well, I finally give up.
I've tried so many variations how to force this function works as a method of "Inhabitant" class, but it doesn't look possible before I initialize "inhabitants" array.
Perhaps, this function should not be the method of main class. I don't know
Or, maybe lot easier resolve exists and it just unclear for me at this point.
Sorry, I remember that this part of exercise does not necessary
We can put whole objects as elements of each "friends" container only after the initialization of our array not before.
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 can call any method after the object initialization. E.g. paul.addFriends([nina, toby, robert])
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.
But in this case, I should totally change the construction of our array, in order the name of inhabitant becomes the name of variable, I guess...
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 can extract names (or any other property) when it is needed exactly.
Consider the following example:
function setParents(parents) { this.parents = parents }
function getParentsNames() { return this.parents.map(parent => parent.name).join(", ") }
Rule of thumb: extract and transform original data exactly where it is needed.
} | ||
|
||
printInhabitant () { | ||
print(`${this.species}; <strong>${this.name}</strong>; ${this.gender}; ${this.limbs.join('; ')}; <em>${this.saying}</em>; ${this.friends ? this.friends.map(elem => elem.name).join(', ') : 'Cats have no 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 are Array
methods to make code DRYer. An array element can be an expression (or rather a result of expression).
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!
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.
Object
methods still do not guarantee order of properties/values.
You could improve exisiting line 22.
Ask Students' chat how it could be improved using Array
methods. You will learn yourself and, probably, other students will learn on this example.
You will come across this pattern quite often. Use the opportunity.
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 think we speak about toString()... I'll try it
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.
Not really. We speak about [].join
and
An array element can be an expression (or rather a result of expression).
Feel free to ask questions
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.
Ok, I think I finally get it, 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.
Could you review it again, please?
Please review again, thank you |
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
Bring back paws and hands (in OOP way), friends and whatever you just removed from the code.
And try to solve this problems. You will learn something useful that you will be able use in your next projects. You will also understand better certain important concepts on Stage 1.
Do not worry about stage 0 deadline.
I bring back and rewrote addFriends function, and make some fixes, please re-review. Thank you |
I've brought legs/hands/paws back, and made some out formatting for greater look at the page. |
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 improvements.
Now need to make the design OOP-compliant.
} | ||
} | ||
|
||
class Animal extends 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.
In fact, every instance of Animal
has property for hands - hands
.
OOP helps to design the system of business entities so that they just do not have unnecessary properties.
This is something bigger than just a way to provide default values to the base class.
Also requirement say that inhabitants that do not have something by natural design they do not have any properties for this. OOP helps to build scalable code that doesn't require to change base class when we want to handle something specific to a sub-class.
Make so that only humans have hands in any meaning. Also remember that very class should handle the properties it owns immediately. Do not delegate property handling to parent class (it should be agnostic of any properties a child class may have).
Learning materials cover this topic. Look for Open-Closed principles in SOLID and all of OOP principles.
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.
Well, I've remade printInhabitant method especially for Human class, for printing out "Hands" property.
This way looks less DRY (Even though rewriting methods is one of the OOP-principles).
Please re-review my fixes. Thank you.
Now Base class and Animal subclass don't have any type of "Hands" property, this became exclusively for Human. |
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 quite a decent job.
Let's make it OOP compliant and use mechanics JS OOP syntax offers to write DRY, maintainable and scalable code.
this.friends = friends; | ||
} | ||
|
||
printInhabitant() { |
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.
(1) Separation of concerns (remember requirements re Frogger?)
This method forms an output and also conducts output.
(2) You may construct arrays from any legal JS expressions. Consider an example below
[
this.propName,
`prefix ${this.PropName} suffix`,
arrayVariable.join("glue "),
condition ? expressionIfTrue : "stringIfFalse",
]
printInhabitant() { | ||
const nameToPrint = `<strong>${this.name}</strong>`; | ||
const legsToPrint = `On ${this.legs} legs`; | ||
const handsToPrint = `Has ${this.hands} hands`; | ||
const talkToPrint = `<em>${this.saying}</em>`; | ||
const friendsToPrint = this.friends.map((friend) => friend.name).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.
Use method overloading/overriding. This will help to make your code DRY.
Check learning materials and find out what super
's extended API can do for you.
Thanks for your review (especially for the hint by extended using "super"). |
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 almost there
} | ||
|
||
printInhabitant() { | ||
super.printToPage(this.species, |
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 not re-use parent's printInhabitant and then adding what is specific to this class?
I see, because parent's printInhabitant
is not really re-usable as it returns nothing.
Use print
function completely outside of classes 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 made it, 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 let's make it OOP compliant in one final step.
return [this.species, | ||
`<strong>${this.name}</strong>`, | ||
this.gender, | ||
`On ${this.legs} legs`, | ||
`Has ${this.hands} hands`, | ||
`<em>${this.saying}</em>`, | ||
this.friends.map((friend) => friend.name).join(", ")].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.
Now re-use parent's version of the method and add whatever this class needs to add.
Let's have hands at the end for the sake of simplicity and in favour of OOP compliance.
Alternative would be to keep properties as array all way down until you need to print them. This way you could insert hands somewhere after legs or elsewhere. But this too much change. Please just keep this in mind. And attach hands at the end of the parent's method output.
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 made it, please review. And sorry for delayed answer, because of missile attacks we don't have an electricity and cable internet.
Thank you
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!
OOP Exercise
Demo |
Code base
The code is submitted in a dedicated feature branch.
Only code files are submitted.
Please, review.