1
\$\begingroup\$

So I am trying to make a Object oriented Node.js class for store information about riddles. Please feel free to be harsh as possible. Would love to hear your ideas about how to make this class better!

function RiddleInfo(riddle, answer, hint1, hint2, hint3, ohFactor, difficulty) {
    this.riddle = riddle || null;
    this.answer = answer || null;
    this.hint1 = hint1 || null;
    this.hint2 = hint2 || null;
    this.hint3 = hint3 || null;
    this.ohFactor = ohFactor || null;
    this.difficulty = difficulty || null;
}

RiddleInfo.prototype.getRiddle = function() {
    return this.riddle;
}

RiddleInfo.prototype.setRiddle = function(riddle) {
    this.riddle = riddle;
}

RiddleInfo.prototype.getAnswer = function() {
    return this.answer;
}

RiddleInfo.prototype.setAnswer = function(answer) {
    this.answer = answer;
}

RiddleInfo.prototype.getHint = function(hintNumber) {
    switch(hintNumber)
    {
        case 1:
            return this.hint1;
            break;
        case 2:
            return this.hint2;
            break;
        case 3:
            return this.hint3;
            break;
        default:
            return 'InvalidArgumentException line 25 in RiddleInfo.js: ' + 
                   'hintNumber must be 1, 2, or 3 but instead was ' + hintNumber;
    }
}

RiddleInfo.prototype.getOhFactor = function() {
    return this.ohFactor;
}

RiddleInfo.prototype.setOhFactor = function(ohFactor) {
    this.ohFactor = ohFactor;
}

RiddleInfo.prototype.getDifficulty = function() {
    return this.difficulty;
}

RiddleInfo.prototype.setDifficulty = function(difficulty) {
    this.difficulty = difficulty;
}

RiddleInfo.prototype.isSimilarTo = function(otherRiddle) {
    if (this.getRiddle() === otherRiddle.getRiddle() ||
        this.getAnswer() === otherRiddle.getAnswer() 
        ) {
        return true;
    } else {
        return false;
    }
}

RiddleInfo.prototype.fill = function(newFields) {
    for (var field in newFields) {
        if (this.hasOwnProperty(field) && newFields.hasOwnProperty(field)) {
            if (this[field] !== 'undefined') {
                this[field] = newFields[field];
            }
        }
    }
};

module.exports = RiddleInfo;
\$\endgroup\$

1 Answer 1

1
\$\begingroup\$

From the outset, I'm not sure how useful/necessary this constructor ("class") is, to be honest. Everything could seemingly be stored in a simple object literal:

var aRiddle = {
  riddle: "What gets wetter and wetter the more it dries?",
  answer: "A towel",
  difficulty: "meh",
  ohFactor: "so-so",
  hints: [
    "Galactic hitchhikers should always bring one",
    "Look in the bathroom",
    "Everyone has several of the things"
  ]
};

Point is, your class doesn't do any input checking, and most of its methods are getters and setters - which don't necessarily make much sense in JavaScript. For instance. I can do this:

var aRiddle = new RiddleInfo();             // no arguments at all: no errors
aRiddle.riddle = "This statement is false"; // bypasses setter completely
aRiddle.foo = 23;                           // arbitrary property assignment
aRiddle.setAnswer(new Date());              // using setter, but passing in nonsense

The only methods left are getHint and isSimilarTo. Both of these could be handled by functions outside the object. Yes, it defies encapsulation, but... 'eh, I'd choose that over a lot of "pointless" code.

But, let's give the code a once-over. Again, I'd suggest a very different approach, but that's not the same as reviewing the code you posted.

First off, your constructor takes an awful lot of arguments. This creates very tight coupling to any other code: All that other code must know the correct order of all those arguments to use the constructor, which makes it harder to change this constructor in the future without breaking everything.
Now, I'm not sure what arguments you consider required and which ones might be optional, but at the very least, I'd say a riddle consists of a question, and an answer. I'd consider the other arguments optional for now. And to avoid having to order the arguments correctly, I'd use an object literal (like the one above) for all the optional arguments:

function RiddleInfo(question, answer, options) {
  // ... see below ...
}

As mentioned, I'd skip getters and setters unless they actually contributing in some way. For instance, they could (should) check their input, to prevent nonsense values being set. And if you're going to have them, you might as well use them yourself.

function RiddleInfo(question, answer, options) {
  this.setQuestion(question);
  // ...
}

RiddleInfo.prototype.setQuestion = function (question) {
  if( typeof question !== "string" ) {
    throw new TypeError("Question must be a string");
  }
  this.question = question;
};

Now the constructor will use the setter, meaning it's guaranteed to throw a TypeError if you're using it wrong. You can do something similar for the other arguments, as well as the optional ones.

Speaking of: Don't return a string that looks like an exception like you do in getHint. It's basically lying: It's not an "InvalidArgumentException" - it's just a string. And hardcoding the line number in there is crazy - you'll have to change it constantly, even if you've only been adding comments or just whitespace. Just throw a real exception. It'll contain a stack trace, and give you the line number.

Anyway, the above neglects one thing: You can still completely bypass the setter by just writing riddle.question = .... Now, as mentioned, getters and setters are probably more trouble than their worth in JavaScript, but if you want a more "secure" way of doing things, you'll have to change things up a bit:

function RiddleInfo(question, answer, options) {
  // define the setter here
  this.setQuestion = function (newQuestion) {
    if( typeof question !== "string" ) {
      throw new TypeError("Question must be a string");
    }
    question = newQuestion;
  };

  // define the getter here too
  this.getQuestion = function () {
    return question;
  };

  this.setQuestion(question);

  // ... similar thing for other arguments ...
}

The point in the above is that question remains a local variable inside the constructor function. Instead of setting it on the instance (this.question = question), we set the getter/setter functions instead. Thanks to closures, they can access the local question variable. What you get is something like

var aRiddle = RiddleInfo("...", "Darkness");

aRiddle.setQuestion("The more there is, the less you see. What could I be?");
aRiddle.getQuestion(); // => "The more there is [...]"
aRiddle.question = "this won't have any effect";
aRiddle.getQuestion(); // => "The more there is [...]"

Lastly, if you've got more than 1 of something, you should probably use an array: i.e. use an array for the hints. You may want more than 3 or fewer, so don't have named variables for exactly 3.

Phew, I think I'll stop here. Hopefully you've got something to chew on.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.