Is this a good JavaScript class?

Question

I'm trying to get my head around object oriented programming, and I'm starting in JavaScript. I have put a working demo of my code on JSFiddle (here), but the class is below.

My main concerns are that as soon as the object is called with a constructor, all the properties are populated straight away (there are no methods). Should I have put some of the contructor code into methods?

function XEString(encryptedString) {
    if (!XEString.test(encryptedString)) { //Throw exception if invalid XEcrypt string
        throw new Error("Invalid XECrypt string");
    } else {

        this.encryptedString = encryptedString; //Save encrypted string to object
        //Remove first "." char and put numbs into array, then convert numbs from strs to ints
        this.encryptedNumbers = this.encryptedString.substring(1).split(".");
        for (var i = 0; i < this.encryptedNumbers.length; i++) {
            this.encryptedNumbers[i] = parseInt(this.encryptedNumbers[i], 10);
        }

        //Each char is calced from a group of 3 numbs
        this.stringLength = this.encryptedNumbers.length / 3;

        //Create array for encrypted chars, and obj to map encrypted char freq
        this.encryptedChars = [];
        this.encryptedCharFreq = {};
        //Loop for each letter
        for (var i = 0; i < this.stringLength; i++) {
            //Add together each group of 3 numbs
            var thisChar = 0;
            for (var j = 0; j < 3; j++) {
                thisChar += this.encryptedNumbers[3 * i + j];
            }
            this.encryptedChars[i] = thisChar; //Store encrypted char code in array
            //Keep a record of how many times each encrypted char occurred
            if (this.encryptedCharFreq[thisChar] == null) { //If this char has not been seen before
                this.encryptedCharFreq[thisChar] = 1; //Set the occurrence in the obj map to 1
            } else { //Otherwise
                this.encryptedCharFreq[thisChar]++; //Increment the occurrence in the obj map
            }
        }

        //Get highest char freq (mode)
        this.highestFreq = 0;
        for (var i in this.encryptedCharFreq) {
            if (this.encryptedCharFreq[i] > this.highestFreq) {
                this.highestFreq = this.encryptedCharFreq[i];
            }
        }

        //Get all highest freq chars (modes) and add objs containing key, decrypted string and numb of unprintable chars
        this.decryptedStrings = [];
        for (var i in this.encryptedCharFreq) {
            if (this.encryptedCharFreq[i] == this.highestFreq) {
                var key = i - 32; //Space char is ASCII code 32
                var decryptedString = "";
                var unprintableChars = 0;
                for (var j = 0; j < this.stringLength; j++) { //for every char add the decoded ascii char and check for unprintable char
                    var decryptedChar = this.encryptedChars[j] - key;
                    decryptedString += String.fromCharCode(decryptedChar);
                    if (decryptedChar < 32 || decryptedChar > 126) { //Printable ASCII char codes range from 32-126
                        unprintableChars++;
                    }
                }
                this.decryptedStrings.push({
                    'key': key,
                    'decryptedString': decryptedString,
                    'unprintablechars': unprintableChars
                });
            }
        }

        //Get least unprintable chars
        this.leastUnprintableChars = Infinity;
        for (i in this.decryptedStrings) {
            if (this.decryptedStrings[i]['unprintablechars'] < this.leastUnprintableChars) {
                this.leastUnprintableChars = this.decryptedStrings[i]['unprintablechars'];
            }
        }

        //Get best decrypted string(s)
        this.bestDecryptedStrings = [];
        for (i in this.decryptedStrings) {
            if (this.decryptedStrings[i]['unprintablechars'] == this.leastUnprintableChars) {
                this.bestDecryptedStrings.push(this.decryptedStrings[i]);
            }
        }
    }
}

//Function for testing valid XEcrypted string. returns true or false
XEString.test = function(encryptedString) {
    return /^((\.-?\d+){3})+$/.test(encryptedString);
}​

Answer 1

It's a tricky one, since your class* doesn't really need any methods. Since its purpose is to decrypt a string, it makes sense to do that decryption straight away. Not much need to call any methods on the instantiated object afterwards.

Besides, I don't see much that would make sense to expose as methods. The decryption steps really only make sense in their current context.

So the way you've done it right now seems appropriate enough, I think.

I guess you could simply skip instantiation, and simply have a decrypt function that returns an object literal (aka map, hash, etc.) with the properties. No need for a constructor and instantiation, just a parseXEString function. That's probably what I'd do.

But if you do want to instantiate you own object, you could keep the code pretty much the same, but add property accessor (reader) methods, and keep the properties themselves "private". This would make the object's properties immutable (whereas now, anyone can say xeObj.decryptedStrings = ... and overwrite the calculated properties.) This can be accomplished like so:

function Demo() {
  var x = 42; // local var

  // make a read accessor for x
  this.getX = function () {
    return x;
  };
}

var d = new Demo;
d.getX() // => 42
d.x;     // => undefined

However, all your code would still need to be inside the constructor in order to be able to access the local variables. I.e. the methods can't be on the prototype.

(And depending on where your code is intended to be used it would be better to use ECMA5's Object.definePropety which let's you add accessors. Of course, older JS runtimes do not support this.)

*) Technically, it's a "constructor" in JS terms, not a class, but that's not terribly important here