Skip to content

Conversation

@synflyn28
Copy link

This is my first attempt at our inheritance homework. I have got the inheritance working, however I couldn't figure out how to refer to the "current class constant" for the "speak" method inherited by the other subtypes of "Animal". I got the test suite to work if I referred to the specific instances of those subtypes (humans, primates, and reptiles) but I couldn't get the suite to pass if I referred to their capital letter constructors.

Copy link
Owner

@joshdmiller joshdmiller left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking pretty good! Aside from the comment on slack about the SOUND bit, I put in a couple comments to bring your code inline with the more "canonical" approach.

src/index.js Outdated
this.thumbs = true;
}
Primate.SOUND = 'Ooh ooh eee!';
Primate.prototype = new Animal();
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're looking for Object.create here. Much cleaner. Otherwise, chlorophyll is defined on the prototype instead of the instance, which isn't in line with what you did with animal. Also see comment above.

src/index.js Outdated
Reptile.prototype.constructor = Reptile;

export function Primate() {
this.thumbs = true;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You'll want to call the Animal constructor within this constructor, and "bind" it to this instance.

@joshdmiller
Copy link
Owner

The diff got outdated as I did the review, so in case you didn't see my other comments, check out the review above. Good work!

@@ -1 +1,2 @@
node_modules/
.idea/
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

<random-thought>
Random comment having nothing to do with this class nor even javascript: you can define a global gitignore file that applies to all repositories, which is a good place for stuff like this. I do it that way so I can omit files that my editor generates without (a) having to change every gitignore in every repo I ever touch; and (b) to prevent committing irrelevant code to repos.
</random-thought>

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't know you could define a global .gitignore across repositories as I mostly just write them per-project. Are you simply just keeping the configurations in a common repository or are you doing any special Github linking between projects?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, it's more "set it and forget it". Check out the link from my first comment. You just keep a file on your computer someplace that looks like a gitignore file and you put its location in your configuration for git (can be done from cli too).

src/index.js Outdated
export function Human(){}
Human.SOUND = 'hello';
Human.prototype = Object.create(Primate.prototype);
Human.prototype.speak = function () {return Human.SOUND};
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about:

Human.prototype.speak = function ( message ) { return message; };

Copy link
Author

@synflyn28 synflyn28 Dec 15, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't look at the unit test carefully for the human type/ES6 Class and that's why I didn't write it this way. This has been fixed here: https://github.com/synflyn28/js-test-boilerplate/commit/cd1b8ff2c7fa303f975995cab3695a1b9c34cffe.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants