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

Bowling scorecard app

Submitted by: @import:stackexchange-codereview··
0
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 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 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 : 0


or currentFrame

currentFrame = 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 : 0
currentFrame = currentFrame < 9 ? currentFrame + 1 : currentFrame;

Context

StackExchange Code Review Q#109221, answer score: 2

Revisions (0)

No revisions yet.