2
\$\begingroup\$

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);
}​
\$\endgroup\$
  • \$\begingroup\$ Could you include code in the question, please? "This site is for code reviews, which are hard to do when the code is behind a link somewhere out there on the internet. If you want a code review, you must post the relevant snippets of code in your question. It is fine to post a "see more" link (though, do be careful — very few reviewers will be willing to click through and read thousands of lines of your code), but the most important parts of the code must be placed directly in the question." (from the FAQ) \$\endgroup\$ – palacsint Oct 9 '12 at 16:05
  • \$\begingroup\$ @palacsint Fair enough (I'm familiar with stackoverflow where some people are happy to debug a live copy of the code), but I understand why it might differ here (I'm new here). I'm not sure how to condense the code for a code review without losing code that I want reviewed, so it is quite long. \$\endgroup\$ – Grezzo Oct 9 '12 at 16:15
1
\$\begingroup\$

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

\$\endgroup\$
  • \$\begingroup\$ Thanks, I think there is some more reading for me to do before I fully understand what you are saying. \$\endgroup\$ – Grezzo Oct 12 '12 at 8:21
  • \$\begingroup\$ @Grezzo Let me know if you have questions, and I'll try my best to answer them (as soon as I have the chance to, anyway) \$\endgroup\$ – Flambino Oct 12 '12 at 8:46

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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