-
-
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 #349
oop-exercise #349
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, |
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.
@hisbvdis quite a decent job.
Let's make it OOP/SOLID compliant.
this.props = [ | ||
{ prop: "species", preface: "I am a" }, | ||
{ prop: "name", preface: "My name is" }, | ||
{ prop: "gender", preface: "I am a" }, | ||
{ prop: "saying", preface: "I can say —" }, | ||
{ prop: "legs", preface: "I have legs:" }, | ||
]; | ||
} | ||
|
||
getProps() { | ||
return this.props | ||
.map(({ prop, preface }) => `${preface} ${this[prop]}`) | ||
.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.
Nice solution to handle props and extra wording for them!
constructor(name, gender, saying) { | ||
super("human", name, gender, saying, 2); | ||
this.hands = 2; | ||
this.props = this.props.concat({prop: "hands", preface: "I have hands:"}); |
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) ES6 offers a different pattern for object literal extension:
obj = {...obj, ...extraProps, ...{yetAnotherProp1: 1, yetAnotherProp2: 2}}
(2) The matter is that class should handle on its own the properties it owns immediately.
There should be good reasons to delegate handling to the base class. There are no such reasons in this task.
Use method overloading and explicit call to an inherited version of the method via extended super
API.
Try to keep the elegancy of property-name/preface approach.
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) ES6 offers a different pattern for object literal extension:
obj = {...obj, ...extraProps, ...{yetAnotherProp1: 1, yetAnotherProp2: 2}}
Коммит: 7f1b42f
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.
(2)...
Use method overloading and explicit call to an inherited version of the method via extendedsuper
API
Не очень хорошо понял этот момент.
Постарался сделать так, как понял.
Проверил через console.log()
— при создании экземпляров вроде вызывается дочерний метод.
Коммит: 5aa89e8
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 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.
(2)...
Use method overloading and explicit call to an inherited version of the method via extendedsuper
APIНе очень хорошо понял этот момент. Постарался сделать так, как понял. Проверил через
console.log()
— при создании экземпляров вроде вызывается дочерний метод.Коммит: 5aa89e8
Yes, and it can call inherited version via super.methodName
and add anything on top of that to build required outcome.
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.
(2)...
Use method overloading and explicit call to an inherited version of the method via extendedsuper
APIНе очень хорошо понял этот момент. Постарался сделать так, как понял. Проверил через
console.log()
— при создании экземпляров вроде вызывается дочерний метод.
Коммит: 5aa89e8Yes, and it can call inherited version via
super.methodName
and add anything on top of that to build required outcome.
Из фразы and add anything on top of that to build required outcome
я понял, что метод дочернего класса должен как-то изменить (дополнить) this.props
родительского класса и вывести их.
К сожалению, приходится гадать.
Может изменять this.props
нужно в дочернем методе?
Коммит: ca97d8d
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.
P.S. Может Вы подумали, что у меня this.props — это объект?
Yes, that was my mistake. Your code is correct in these regards.
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.
Из фразы and add anything on top of that to build required outcome я понял, что метод дочернего класса должен как-то изменить (дополнить) this.props родительского класса и вывести их.
Rather add. Changing properties that a class doesn't own immediately is in general a bad practice.
In this specific case changed props
property will not be used by the child class. Instead the base class will handle child's property (to create an output string) it knows nothing and shouldn't know nothing about, implicitly extending base class functionality. Extension should only work one way: parent to child in all regards.
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.
Imagine we want to change how hands
are reprsented internally (e.g. we want to store an object denoting count of left and right hands instead of simple number), still output would be a simple count of all hands. If we do this change we then are forced to change the base class (conversion of hands
property into a number of hands). This breaks encapsulation and OOP principles. Besides we may not be allowed to change the base class at all.
So, a that owns a property should handle it on its own, end to end. Child classes may borrow (via inheritance or explicit calls of inherited versions) anything from base classes chain, but never from child to any of parents in the inheritance chain.
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.
Может так?
Я Вызываю родительский метод и дополняю его данными из дочернего класса.
P.S. Мне очень жаль, что я не могу понять задачу. Видимо, не хватает понимания ООП.
Коммит: ce36f35
class Animal extends Creature { | ||
constructor(species, name, gender, saying) { | ||
super(species, name, gender, saying); | ||
this.species = 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?
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.
Да, действительно.
Значение из конструктора сразу идёт в super()
Удалил лишнюю строку
Коммит: db1853d
const woman = new Woman("Bella", "female"); | ||
const man = new Man("Vasya", "male"); |
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.
Add 3 more humans.
Allow humans to have unique 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.
Коммит: 832570d
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.
@hisbvdis well done!
Let's polish it. Also read through your code to check if anything else should be fixed.
super(species, name, gender, saying); | ||
this.legs = 4; |
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.
Classes shouldn't assign values to properties they do not own.
Besides Creature
's constructor has parameter for this.
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.
Исправил
Коммит: a20d954
Исправил последовательность присваивания свойств, чтобы совпадало с последовательностью в конструкторе Коммит: e0d6492
Кроме перестановки последовательности строк с присваиванием свойств больше ничего не нашёл |
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.
@hisbvdis great job!
oop-exercise
Demo |
Code base
The code is submitted in a dedicated feature branch.
Only code files are submitted.
Please, review.