HiveBrain v1.2.0
Get Started
← Back to all entries
patternjavascriptModerate

Self-playing Baseball game

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
playinggamebaseballself

Problem

The Demo for what I have made is located here

Basically I am trying to make a self playing JavaScript baseball game with the use of no libraries.

Is there anything I could improve on in the code?

```
var writeDown = {
delay: 110,
add: null,
div: document.getElementById('playArea'),
log: function() {
var args = arguments;
if (args.length == 0) {
args = [''];
}
var div = this.div;
setTimeout(function() {
//console.log(args[0]);
div.innerHTML = args[0] + "" + div.innerHTML;
}, this.delay);
if (this.add == null) {
this.add = this.delay;
}
this.delay += this.add;
},

updateDiv: function(msg, div) {
setTimeout(function() {
//console.log(msg, div);
div.innerHTML = msg;
}, this.delay);
},

updateDiv_delay: function(msg, div) {
setTimeout(function() {
document.getElementById(div).innerHTML = msg;
}, this.delay);
},

updateDiv_color: function(color, div) {
setTimeout(function() {
//console.log(color, div);
document.getElementById(div).style.background = color;
}, this.delay);
}
}

var Diamond = function(d_name, d_id) {

var name = d_name;
var diamond = document.getElementById(d_id);

var bases = {
first: false,
second: false,
third: false,
home: false
};

var players = {
first: false,
second: false,
third: false,
home: false
};

this.clear = function() {
bases = {
first: false,
second: false,
third: false,
home: false
};
players = {
first: false,
second: false,
third: false,
home: false
};
this.updateBases();
writeDown.updateDiv('', diamond);
}

this.onBase = functio

Solution

var writeDown = {
    delay: 110,
    add: null,
    div: document.getElementById('playArea'),
    log: function() {
        var args = arguments;
        if (args.length == 0) {
            args = [''];
        }
        var div = this.div;
        setTimeout(function() {
            //console.log(args[0]);
            div.innerHTML = args[0] + "" + div.innerHTML;
        }, this.delay);


What are you doing with the arguments? You seem to be taking a roundabout method to have log optionally take a single parameter. But it's not clear why logging without any arguments would make any sense.

if (this.add == null) {
            this.add = this.delay;
        }
        this.delay += this.add;


You've done this by scheduling a series of events to happen later and later from now. That's kinda clever but not something that you should do in a JavaScript context. A better way would be to structure your code so that a single function is called repeatedly to perform logic so that the processing is ongoing rather than done all upfront.

},

    updateDiv: function(msg, div) {
        setTimeout(function() {
            //console.log(msg, div);
            div.innerHTML = msg;
        }, this.delay);
    },

    updateDiv_delay: function(msg, div) {
        setTimeout(function() {
            document.getElementById(div).innerHTML = msg;
        }, this.delay);
    },


These very similar functions as well as the similar code above is begging to be combined.

updateDiv_color: function(color, div) {
        setTimeout(function() {
            //console.log(color, div);
            document.getElementById(div).style.background = color;
        }, this.delay);
    }
}

var Diamond = function(d_name, d_id) {

    var name = d_name;
    var diamond = document.getElementById(d_id);

    var bases = {
        first: false,
        second: false,
        third: false,
        home: false
    };


When elements have a sequential interpretation, you are usually better of using numbers rather than names. Bases should be an array not an object.

var players = {
        first: false,
        second: false,
        third: false,
        home: false
    };


Instead of storing players like this, put everything in the bases array. Use null or false for no player, and the player's name otherwise.

this.clear = function() {
        bases = {
            first: false,
            second: false,
            third: false,
            home: false
        };
        players = {
            first: false,
            second: false,
            third: false,
            home: false
        };
        this.updateBases();
        writeDown.updateDiv('', diamond);
    }


You should use the clear function inside this constructor so that you don't have to duplicate its contents.

this.onBase = function(base_amt, PlayerName) {


Either use variable_like_this or VariablesLikeThis or variablesLikeThis. Avoid mixing styles. Also base_amt isn't really clear what it means.

var return_runs = 0;
        switch (base_amt) {
        ...


Your code to determine the new bases and runs is way too complicated. Your code should look something like:

for each base
      if man on base, move forward base_amt places
             if move is past home, score a run
             otherwise set new position


You should never need to write such repetitive code as you wrote above.

var man_on = "",
            base_names = ['first', 'second', 'third'];

        for (var i = 0; i ";
            }
        }


If you take my advice about using arrays instead of objects for keeping track of bases this code will be much simpler.

writeDown.updateDiv(man_on, diamond);
        this.updateBases();
        return return_runs;
    }

    this.updateBases = function() {
        for (base in bases) {
            if (bases[base] == true) {


Don't == true, just use if(bases[base]

writeDown.updateDiv_color('#F00', base);
            }
            else {
                writeDown.updateDiv_color('#AAA', base);
            }
        }
    }

    this.playGame = function(TeamA, TeamB, innings) {
        var score_div = document.getElementById('score');
        writeDown.updateDiv(TeamA.name + ": " + TeamA.getScore() + "" + TeamB.name + ": " + TeamB.getScore() + "" + "Outs: 0" + "Inning: 1", score_div);

        for (var i = 0; i INNING " + (i + 1) + "");
            writeDown.updateDiv_delay("Top of " + (i + 1), 'inning');
            if (TeamA.teamUp()) {


As far as I can tell teamUp never returns false. Its also deceiving because I'd expect something like that in an if to be answering a question not running a complete half-inning. Neither the name nor how it's used hint that the function does that.

```
writeDown.updateDiv_delay("Bottom of " + (i + 1), 'inning');
writeDown.log(TeamA.name + " are out ");
this.clear();
TeamA.resetOuts();

Code Snippets

var writeDown = {
    delay: 110,
    add: null,
    div: document.getElementById('playArea'),
    log: function() {
        var args = arguments;
        if (args.length == 0) {
            args = [''];
        }
        var div = this.div;
        setTimeout(function() {
            //console.log(args[0]);
            div.innerHTML = args[0] + "<br/>" + div.innerHTML;
        }, this.delay);
if (this.add == null) {
            this.add = this.delay;
        }
        this.delay += this.add;
},

    updateDiv: function(msg, div) {
        setTimeout(function() {
            //console.log(msg, div);
            div.innerHTML = msg;
        }, this.delay);
    },

    updateDiv_delay: function(msg, div) {
        setTimeout(function() {
            document.getElementById(div).innerHTML = msg;
        }, this.delay);
    },
updateDiv_color: function(color, div) {
        setTimeout(function() {
            //console.log(color, div);
            document.getElementById(div).style.background = color;
        }, this.delay);
    }
}


var Diamond = function(d_name, d_id) {

    var name = d_name;
    var diamond = document.getElementById(d_id);

    var bases = {
        first: false,
        second: false,
        third: false,
        home: false
    };
var players = {
        first: false,
        second: false,
        third: false,
        home: false
    };

Context

StackExchange Code Review Q#2631, answer score: 13

Revisions (0)

No revisions yet.