3
\$\begingroup\$

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 < 3)) return false

    return (rolls == 2 || players[playerNumber].frames[currentFrame][0] == 10) ? true : false 
}

function nextFrame() {
    if (currentFrame < 9) {
        currentFrame++
    }
}

function nextPlayer() {
    if(playerNumber < (players.length - 1)){
        playerNumber ++
    } else {
        playerNumber = 0
    }
}

this.playerRolled = function playerRolled(pins) {
    if (!gameOver) {
        // validate pins not more than 10

        if (players[playerNumber].frames[currentFrame]) {
            players[playerNumber].frames[currentFrame].push(pins)
        } else {
            players[playerNumber].frames[currentFrame] = [pins]
        }

        frameCompleted = frameFinished();
        isLastPlayer = (playerNumber == (players.length - 1))

        if (frameCompleted) {
            if (isLastPlayer) nextFrame();
            nextPlayer();
        } 

        gameOver = isGameOver()
        return true
    }

    return false
}   

this.getScoreCard = function getScoreCard() {
    for (var player in players) {
        var runningTotals = calculateFrameTotals(player)
        players[player].scores = runningTotals
    }
    return players;
}

function calculateFrameTotals(playerNumber) {
    var sum = 0;
    var sumFrameRolls = 0;
    var totalPointsPerFrame = []

    function getSum(startingFrame, length) {
        // console.log('start', startingFrame, players[playerNumber].frames[startingFrame])
        for (var num in players[playerNumber].frames[startingFrame]) {
            sum += players[playerNumber].frames[startingFrame][num];
            length--;
            if (length==0) break;
        }
        if (length > 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[playerNumber].frames[frame].map(function(item){
            sumFrameRolls += item
        })

        if ( players[playerNumber].frames[frame][0] == 10 ) {
            getSum(parseInt(frame)+1, 2)
        } else if (sumFrameRolls == 10) {
            getSum(parseInt(frame)+1, 1)
        }

        totalPointsPerFrame.push(sumFrameRolls + sum) 
    }

    return totalPointsPerFrame;
}
};

Then to run the app, let's say using a Node console:

var game = new BowlingGame(['Ron Buenavida','Omer'])
game.playerRolled(10);
game.playerRolled(3);
game.playerRolled(2);
game.playerRolled(10);
console.log(JSON.stringify(game.getScoreCard()))
\$\endgroup\$
2
\$\begingroup\$

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.

\$\endgroup\$
1
  • \$\begingroup\$ Tthank you so much for your feedback @Joseph-the-Dreamer. Makes lot's of sense. I definitely learned something for my future javascript developments. One small question if you have some time, what do you think about public method playerRolled? \$\endgroup\$ – Ron Buenavida Oct 30 '15 at 13:06

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.