patternjavascriptMinor
Bowling scorecard app
Viewed 0 times
bowlingscorecardapp
Problem
This is one of my first attempts in writing a bowling scorecard app. It can have more than one player. I would like to know if anyone has suggestions to improve the code. I am trying to make it as object oriented and as organized as possible. There are 2 public methods, one that accepts pins that were knocked down, and another method that gets score card information that can later be displayed to the user.
I feel though for some reason that the
Is this a good design?
```
var BowlingGame = function(params) {
var currentFrame = 0;
var playerNumber = 0;
var gameOver = false;
var players = [];
if (params) {
for(var name in params) {
players.push({"name": params[name], frames : []})
}
}
function isLastFrame() {
return (currentFrame == 9)
}
function isGameOver() {
return (isLastFrame() && frameFinished() && (playerNumber == (players.length - 1)))
}
function frameFinished() {
if (!players[playerNumber].frames[currentFrame]){
return false
}
var rolls = players[playerNumber].frames[currentFrame].length
var sumRolls = 0;
players[playerNumber].frames[currentFrame].map(function(item){
sumRolls += item
})
if (isLastFrame() && (sumRolls >= 10) && (rolls 0 && players[playerNumber].frames[startingFrame+1]) {
getSum(startingFrame + 1, length)
}
}
// for each frame
for (var frame in players[playerNumber].frames) {
sum = 0
sumFrameRolls = 0
players[player
I feel though for some reason that the
playerRolled method is a little bit disorganized. Maybe it's responsible for too many things. Essentially in mind I wanted to create a simple way to interface with the object by just having one method that does everything, will add points, change players, change frames, and returns boolean if game is completed. This leaves all the game logic within the class and users of this class just have to feed the application some player names and the pins being knocked. The app will take care of the rest.Is this a good design?
```
var BowlingGame = function(params) {
var currentFrame = 0;
var playerNumber = 0;
var gameOver = false;
var players = [];
if (params) {
for(var name in params) {
players.push({"name": params[name], frames : []})
}
}
function isLastFrame() {
return (currentFrame == 9)
}
function isGameOver() {
return (isLastFrame() && frameFinished() && (playerNumber == (players.length - 1)))
}
function frameFinished() {
if (!players[playerNumber].frames[currentFrame]){
return false
}
var rolls = players[playerNumber].frames[currentFrame].length
var sumRolls = 0;
players[playerNumber].frames[currentFrame].map(function(item){
sumRolls += item
})
if (isLastFrame() && (sumRolls >= 10) && (rolls 0 && players[playerNumber].frames[startingFrame+1]) {
getSum(startingFrame + 1, length)
}
}
// for each frame
for (var frame in players[playerNumber].frames) {
sum = 0
sumFrameRolls = 0
players[player
Solution
First, don't use
In
Further down in the same function, I see you have a bunch of conditions. You can actually just combine them. Additionally, conditions are by themselves boolean. No need to use a ternary to return
Now one problem with
or
The rest of your code seem to follow the same pattern. I suggest applying what I have reviewed to the rest, where applicable.
for-in for arrays. It will run through the elements of the array as well as other properties. Use a regular for loop, or better use map instead. Additionally, I suggest you name params to something else. params is too generic, plus the array isn't really "params". It's a list of player names.var players = playerNames.map(function(player){
return { name: player, frames : []}
});In
frameFinished, I see you use map to construct a sum. reduce is the better method for such operation.var sumRolls = players[playerNumber].frames[currentFrame].reduce(function(sum, item){
return sum + item;
}, 0);Further down in the same function, I see you have a bunch of conditions. You can actually just combine them. Additionally, conditions are by themselves boolean. No need to use a ternary to return
true or false. Also, I suggest you put the values in variables for easy comprehension.var isLastFrame = isLastFrame();
var hasRolledAllTenRounds = sumRolls >= 10;
var hasRolledLessThanThree = rolls < 3;
var hasRolledTwo = rolls === 2;
var currentPlayerIsAtFrameTen = players[playerNumber].frames[currentFrame][0] === 10;
return !(isLastFrame && hasRolledAllTenRounds && hasRolledLessThanThree) || // and so on...Now one problem with
if statements is in the long run, they can easily run out of control and end up in deeply nested situations. One way you can avoid that is to use ternaries and condition variables. For instance, nextPlayer.playerNumber = (playerNumber < (players.length - 1)) ? player + 1 : 0or
currentFramecurrentFrame = currentFrame < 9 ? currentFrame + 1 : currentFrame;The rest of your code seem to follow the same pattern. I suggest applying what I have reviewed to the rest, where applicable.
Code Snippets
var players = playerNames.map(function(player){
return { name: player, frames : []}
});var sumRolls = players[playerNumber].frames[currentFrame].reduce(function(sum, item){
return sum + item;
}, 0);var isLastFrame = isLastFrame();
var hasRolledAllTenRounds = sumRolls >= 10;
var hasRolledLessThanThree = rolls < 3;
var hasRolledTwo = rolls === 2;
var currentPlayerIsAtFrameTen = players[playerNumber].frames[currentFrame][0] === 10;
return !(isLastFrame && hasRolledAllTenRounds && hasRolledLessThanThree) || // and so on...playerNumber = (playerNumber < (players.length - 1)) ? player + 1 : 0currentFrame = currentFrame < 9 ? currentFrame + 1 : currentFrame;Context
StackExchange Code Review Q#109221, answer score: 2
Revisions (0)
No revisions yet.