OOP Exercise #608
OOP Exercise #608
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, |
|
Self-check is complete. |
OleksiyRudenko
added
Stage0.1
self-check-done
| } | ||
|
|
||
| toString() { | ||
| const {constructor: {name: species}, name, gender, legs, friends} = this; |
There was a problem hiding this comment.
How about setting species in constructor? it is definitely a property of a creature that exists not only when we want to learn anything about a creature.
There was a problem hiding this comment.
I misunderstood the requirement in "Typical mistakes" section: "1.iv : There is no need to specify species at instantiation yet species are printed". Thought that I must avoid to create property species in general. Fixed.
There was a problem hiding this comment.
at instantiation is when we do new ClassDefinition. Whatever is within constructor is already the initialization phase.
There was a problem hiding this comment.
Now I see. Just was confused
| const [species, name, gender, legs, friends] = creatureString.split('; '); | ||
| return [species, name, gender, legs, hands, phrase, friends].join('; '); |
There was a problem hiding this comment.
I see what you are doing here.
In OOP paradigm child classes should be unaware of how things are built in any base class.
If we want to add yet another shared property to the base class, we will need to change toString implementation in many child classes. We do not want this.
You do not need to fix this now.
There was a problem hiding this comment.
Now I see that I implemented indeed not good solution.
I wanted to keep initial order of properties, but hands are to be inserted into the middle of presentation string. So the main question, how to find the place in this string? Parsing comes to mind as the more common approach but at he same time as very complicated. What would be better to do in such cases?
There was a problem hiding this comment.
Keep a map (list) of properties rather than converted string and use semantics like insertPropertyAfter(property, target) or insertBefore...
Do the list => string conversion at the very last moment. Outside any class definition in this case.
The closer the class to a definition of pure function the better. Avoid terminal functions (i.e. those that return nothing / return undefined), and functions that interact with any scope outside their own (class scope for methods also comprises "their own") as much as possible.
There was a problem hiding this comment.
Ok, thank you!
There was a problem hiding this comment.
When I added phrase to base class the toString() turned into simple super.toString() everywhere except in Human class. This is only class I leave as it is.
There was a problem hiding this comment.
@daniil-bodryagin Oh, I missed this one. You do not need to overload a method in child class if it's only function is to call an inherited version of the method. It is inherited by child classes and available out of the box by default.
| } | ||
|
|
||
| toString() { | ||
| const {phrase} = this; |
There was a problem hiding this comment.
phrase is obviously a shared property just as legs and or name or friends.
Why treated differently?
There was a problem hiding this comment.
In the Cat and Dog classes phrase is the only property which is absent in base class. In Human class they are hands and phrase respectively.
Why didn't I put this property in the base class? Because every cat must say the same - "Meow" (as it was written in the assignment), and if it would be part of base constructor I would be able to pass different phrases as arguments every time I create new class instance.
Why didn't I put this property in child classes with predefined value? I wanted to do this, but the obstacle is Catwoman class. I needed to borrow phrase property from Cat that's why I created special functions just as canMeow() which sets the phrase value to cats and catwomen. Since humans can say different phrases they have this property in constructor.
Why didn't I use function to implement composition only for catwoman and didn't leave cats and dogs with phrase property in costructors? It seemed to me that code would look inconsistent then and that it would be right to use special function for adding phrase property everywhere.
There was a problem hiding this comment.
Why didn't I put this property in the base class? Because every cat must say the same - "Meow" (as it was written in the assignment), and if it would be part of base constructor I would be able to pass different phrases as arguments every time I create new class instance.
This is solvable by sending a relevant value to the parent's constructor.
Why didn't I put this property in child classes with predefined value? I wanted to do this, but the obstacle is Catwoman class. I needed to borrow phrase property from Cat that's why I created special functions just as canMeow() which sets the phrase value to cats and catwomen. Since humans can say different phrases they have this property in constructor.
... and that can be canMeow() (probably a bit refactored), i.e. super(.... canMeow()...) is a valid approach.
Also you may want to look at static class methods to solve the problem with CatWoman's phrase. Cat.getPhrase() is even more expressive than canMeow from the global context.
canMeow is the right approach for compositional pattern (as opposed to hierarchical approach of OOP). That's exactly the catch here.
There was a problem hiding this comment.
Added phrase to base class, replaced global functions by static class methods. Now the code looks better indeed. Thanks!
…bal functions were replaced by static class methods.
OOP Exercise
Demo |
Code base
The code is submitted in a dedicated feature branch.
Only code files are submitted.
Please, review.