patternjavascriptModerate
Self-playing Baseball game
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
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 positionYou 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.