4
\$\begingroup\$

This is my first attempt at writing something in JavaScript besides a few Project Euler problems I did first. It's intended to be a library used client side and server side (both of which I know very little about). I've read Crockford's Javascript: The Good Parts, but nothing else. I have the most current experience with Python.

I'd appreciate any feedback (especially style concerns), I know there's a terrifically large amount of stuff to address, so if that's overwhelming, I have these immediate concerns:

  • By using closure with my country object, am I wasting memory, and hence should be using prototypical inheritance?
  • My JSONify code seems redundant, I assume there's a more correct way to do this?
  • Mixing private instance variables like "countries" and public instance variables looks ugly. Is that how it's supposed to be done?

Full code available, link is to the commit which brings that code up to date with what I've posted here. There's a larger object (250 lines) I would love to post if that's not excessive, but for now here's the smaller object. (50 lines)

var country = function(name, connected_to){
    // variables in constructor signature are private
    // These are private variables that weren't in the constructor signature
    var helper = function(player, num_troops){
        // These are public methods
        return {
            'get_name' : function(){return name},
            'get_troops' : function(){return num_troops;},
            'get_owner' : function(){return player;},
            'get_touching_names' : function(){return connected_to},
            'is_owned_by' : function(in_player){return in_player == player},
            'is_touching' : function(to){
                if (!(typeof to === 'string')){
                    to = to.get_name();
                }
                for (var i = 0; i < connected_to.length; i++){
                    if (connected_to[i] === to){
                        return true;
                    }
                }
                return false;
            },

            'set_state' : function(in_player, in_num_troops){
                player = in_player;
                num_troops = in_num_troops;
            },
            'toString' : function(){
                return '[country '+name+']';
            },
            'get_ascii' : function(){
                return name + ' owned by ' + player + ' with '+ num_troops +
                    '\n        connected to ' + connected_to;
            },
            'jsonify' : function(){
                var j = {
                    'name' : name,
                    'player' : player,
                    'num_troops' : num_troops,
                    'connected_to' : connected_to
                }
                return j;
            },
            'dejasonify' : function(j){
                name = j.name;
                player = j.player;
                num_troops = j.num_troops;
                connected_to = j.connected_to;
            }
        };
    };
    return helper(null, null)
};
\$\endgroup\$
4
\$\begingroup\$

If you stop worrying about "privates" everything becomes much cleaner.

By using closure with my "country" object am I wasting memory, and hence should be using prototypical inheritance?

The closure is a small performance hit. For a board game it won't matter. But if you decide not to use local variables in closures (AKA "privates") then you can get rid of the closure and use a constructor function / prototype.

One benefit of doing that is that it will give tools like eclipse, webkit inspector, etc. a clue as to how things are represented in your code so they can annotate stuff for you, provide pop-up help, autocompletion, things like that.

My jsonify code seems redundant, I assume there's a more correct way to do this?

Mixing private instance variables like "countries" and public instance variables looks ugly, is that how it's supposed to be done?

Getting rid of "privates" and making these all properties would be the easiest way to fix both of these issues. In that case the get_* functions could also go.

A few other points:

  • Usually javascript identifiers are written as camelCase, not underscore_separated.

  • The quotes around the property names in the object literal are not needed for character sequences that would otherwise be valid identifiers.

  • to is a confusing variable name, try to find something more descriptive. I changed it to country in the example below.


I would probably write it something like this:

function Country (name, connectedTo){
    this.name = name;
    this.connectedTo = connectedTo;
    // other constructor stuff...
}

Country.prototype = {
    
    // default properties
    
    numTroops: 0,

    player: null,
    
    // functions
    
    isOwnedBy: function(inPlayer){
        return inPlayer == this.player;
    },
    isTouching: function(country){
        // best to avoid typeof if possible
        if (country.name){ 
            country = country.name;
        }
        // this performs somewhat better than evaluating foo.length every iteration
        for (var i = 0, len = this.connectedTo.length; i < len; i++){
            if (this.connectedTo[i] === country){
                return true;
            }
        }
        return false;
    },
    setState: function(inPlayer, inNumTroops){
        this.player = inPlayer;
        this.numTroops = inNumTroops;
    },
    toString: function(){
        return '[country '+this.name+']';
    },
    getAscii: function(){
        return this.name + ' owned by ' + this.player + ' with '+ this.numTroops +
            '\n        connected to ' + this.connectedTo;
    }
};

If setState is only called once per object, you might consider making that code part of the constructor and getting rid of setState.

\$\endgroup\$
6
  • \$\begingroup\$ Thanks! Coming from Python, I can certainly get into not worrying about keeping things private. \$\endgroup\$
    – Thomas
    Jan 12 '12 at 2:30
  • \$\begingroup\$ Yeah, in general you shouldn't worry about it. You could use this approach together with a closure if you want, just return Country from the closure and assign the result to a variable with the same name. But you'd generally only do that if you were building an API or something where you wanted to keep some things out of your objects to avoid confusing users. Of course, functional programming (essentially functions returning functions) is another case where you need closures. \$\endgroup\$
    – Dagg
    Jan 12 '12 at 3:04
  • \$\begingroup\$ I'll continue to use closure with my larger object I didn't post which implements the game, but it'll be prettier now thanks to this answer. \$\endgroup\$
    – Thomas
    Jan 12 '12 at 3:34
  • \$\begingroup\$ Ah, and I'm use JSON.stringify instead of a custom jsonify method. \$\endgroup\$
    – Thomas
    Jan 12 '12 at 3:47
  • \$\begingroup\$ Yeah, but your jsonify didn't convert it to a json string... I thought you were just preparing an object to be stringified by JSON.stringify. But doing it this way you don't need to prepare an object, because instances of Country already have all the properties you want, so you can just stringify them. \$\endgroup\$
    – Dagg
    Jan 12 '12 at 4:24

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.