Skip to content
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

Tests for "Make a Person" bonfire don't make sense #1395

Closed
LimeBlast opened this issue Jul 27, 2015 · 2 comments
Closed

Tests for "Make a Person" bonfire don't make sense #1395

LimeBlast opened this issue Jul 27, 2015 · 2 comments

Comments

@LimeBlast
Copy link

I think there might be something wrong with the Make a Person bonfire, as what I've found as a passing solution goes against everything that I'd just learnt in the Udacity tutorials.

Using the following solution, I'm able to get all of the tests passing, except for the first:

var Person = function(firstAndLast) {
  this.setFullName(firstAndLast);
};

Person.prototype.getFirstName = function() {
  return this.first;
};

Person.prototype.getLastName = function() {
  return this.last;
};

Person.prototype.getFullName = function() {
  return this.first + ' ' + this.last;
};

Person.prototype.setFirstName = function(first) {
  this.first = first;
};

Person.prototype.setLastName = function(last) {
  this.last = last;
};

Person.prototype.setFullName = function(firstAndLast) {
  names = firstAndLast.split(' ');
  this.first = names[0];
  this.last = names[1];
};

var bob = new Person('Bob Ross');
bob.getFullName();

I'm no expect at this, and I'm sure my code could probably be slightly better refactored, but based on what udacity has just been teaching, this seems like a fairly good solution - except that it fails on the first test, the one stating expect(Object.keys(bob).length).to.eql(6); should be true;

So I've spent a bit of time looking at other solutions, such as this one, and the one listed here, which both satisfy the test condition, but in an inefficient way.

Anyway, I could be missing something, if so, please let me know what. Thank you.

@clintoncampbell
Copy link

I ran into the same issue, and I don't think the problem aligns well with the methodology Udacity taught.

I think they're approaching it this way to prevent you from directly accessing bob.first or bob.last. I think the way you accomplish this is to take advantage of closures by pulling the functions into Person() and declaring local variables rather than properties of the object.

So code starts out like so:

var Person = function(firstAndLast) {
var first, last;

var setFullName = function(firstAndLast) {
  names = firstAndLast.split(' ');
  first = names[0];
  last = names[1];
};

I imagine you could also do this.

var Person = function(firstAndLast) {
var first, last;

Person.prototype.setFullName = function(firstAndLast) {
  names = firstAndLast.split(' ');
  first = names[0];
  last = names[1];
};

However, I think you'll fail the property count check. I don't think you prevent the functions being recreated every time either.

It'd be cool if the trickier Bonfires opened up an extra Waypoint to discuss issues like this and alternate implementations.

@BerkeleyTrue
Copy link
Contributor

closing in favor of #881

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

No branches or pull requests

3 participants