Risk board game - best practices, private vs public variables and methods

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;
}

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.

StackExchange Code Review Q#7696, answer score: 4