Code Review Stack Exchange is a question and answer site for peer programmer code reviews. Join them; it only takes a minute:

Sign up
Here's how it works:
  1. Anybody can ask a question
  2. Anybody can answer
  3. The best answers are voted up and rise to the top

This is my first time writing a basic program/game in any programming language. It is supposed to a be a Nim game vs a computer with these basic parameters:

  1. Use prompt() to prompt moves and input.
  2. Use console.log() draw the board.

The board should look like this:

Pile A: ooooo

Pile B: ooo

Pile C: ooooo

It has been very difficult for me to approach this problem, and I know the logic that I have used is very basic and buggy. Any pointers would be appreciated, whether in the code or my approach to solving the problem in general.

 var pileA = {
   circles: "ooooo",
   print: function() {
     console.log("Pile A: " + this.circles);
   }
 };

 var pileB = {
   circles: "ooooo",
   print: function() {
     console.log("Pile B: " + this.circles);
   }
 };

 var pileC = {
   circles: "ooooo",
   print: function() {
     console.log("Pile C: " + this.circles);
   }
 };

 function printBoard(){
     console.log("**** Nim ****");
     pileA.print();
     pileB.print();
     pileC.print();
 }

 function yourMove(){

     printBoard();

     var moveData = prompt("Type letter of pile and # of o's you are removing.\n" +
                             "Ex. A1 or C4");

     if (moveData[0] == 'A') {
         var x = pileA.circles.length - moveData[1];
         pileA.circles = pileA.circles.slice(0,x);
         return pileA.circles;
     } else if (moveData[0] == 'B') {
         var x = pileB.circles.length - moveData[1];
         pileB.circles = pileB.circles.slice(0,x);
         return pileB.circles;
     } else if (moveData[0] == 'C') {
         var x = pileC.circles.length - (moveData[1]+1);
         pileC.circles = pileC.circles.slice(0,x);
         return pileC.circles;
         }

 }

 function computersMove() {

     printBoard();

     alert("Now it is the computer's turn!");

     if (pileA.circles != "") {
         pileA.circles = pileA.circles.slice(0,0);
         return pileA.circles;
     } else if (pileB.circles != "") {
         pileB.circles = pileB.circles.slice(0,0);
         return pileB.circles;
     } else if (pileC.circles != "") {
         pileC.circles = pileC.circles.slice(0,0);
         return pileC.circles;
     }

     if (pileA.circles == "" &&
     pileB.circles == "" &&
     pileC.circles == "") {
         console.log("Computer Wins!!!");
         }
 }

 while (pileA.circles != "" || 
     pileB.circles != "" ||
     pileC.circles != "") {

         yourMove();
         if (pileA.circles == "" &&
             pileB.circles == "" &&
             pileC.circles == "") {
                 console.log("You win!!!");
                 }

        if (pileA.circles != "" || 
            pileB.circles != "" ||
            pileC.circles != "") { 
                 computersMove();
                 }
     }

 alert('Game Over!');
share|improve this question
up vote 2 down vote accepted

First off, I think you've done a great job for your first project! However, as a beginner, you have quite a few things you could improve. I plan to focus on only one thing in this review and that is to...

Eliminate Redundant Code

Use an array of piles instead

Because your piles each have their own variable, you'll need to change a lot of your code if you wanted to add or remove piles. Also, it can get really redundant when you do the same thing over and over with each of your piles:

pileA.print();
pileB.print();
pileC.print();
// ...
if (pileA.circles != "") {}
if (pileB.circles != "") {}
if (pileC.circles != "") {}

You could shorten, simplify, and increase the power your code by using an array of piles instead.

var piles = [
  {name: "A", circles: "ooooo"},
  {name: "B", circles: "ooooo"},
  {name: "C", circles: "ooooo"}
];

Instead of giving each pile a member function to print merge them into one:

function printPile(pile) {
  console.log("Pile " + pile.name + ": " + pile.circles);
}

function printBoard() {
  console.log("**** Nim ****");
  piles.forEach(printPile);
}

Note how Array.prototype.forEach was used. This is a powerful high order function that can be used to preform an action on each element of an array. You could also use a for loop to print each pile:

function printBoard() {
  console.log("**** Nim ****");
  for (var i = 0; i < piles.length; i++) {
    printPile(piles[i]);
  }
}

However, the for loop is much longer and has a lot of boiler plate code that distracts from the purpose. The two code blocks read as follows:

forEach function: call printPile on each element of `piles

for loop: Start a variable i at 0. Then count it up to (but not including) piles.length. Count by increments of 1. For each count call printPile on the ith element of piles

Also note that some of your other code depends on your pile variables, so it will need to be changed if you decide to use the array format. I give examples of how it can be done in the next section using a couple other extremely useful functions:

In the end, not only does the array format decrease the code size, it also vastly simplifies the process of adding or removing piles. You only need to change the piles array:

var piles = [
  {name: "A", circles: "ooooo"},
  {name: "B", circles: "ooooo"},
  {name: "C", circles: "ooooo"},
  {name: "D", circles: "ooooo"},
  {name: "E", circles: "ooooo"},
  {name: "F", circles: "ooooo"},
];

The rest of your code knows how to handle it.

Break up yourMove and computersMove

There's quite a bit of redundancy in yourMove and computersMove. The only real difference is in how each choose which move to make. You could break up the differences and merge the similarities of the two methods like so:

function getUserMove() {
  return prompt("Type letter of pile and # of o's you are removing.\n" +
    "Ex. A1 or C4");
}

function getComputerMove() {
  var chosenPile = piles.find(function(pile) {
    return pile.circles !== "";
  });

  return chosenPile.name + chosenPile.circles.length;
}

function makeMove(move) {
  var chosenPile = piles.find(function(pile) {
    return pile.name === move[0];
  });

  var numLeft = chosenPile.circles.length - parseInt(move[1]);

  chosenPile.circles = chosenPile.circles.slice(0, numLeft);
}

function isGameOver() {
  return piles.reduce(function(isOver, pile) {
    return isOver && pile.circles === "";
  }, true);
}

Once again, eliminating the redundancy does more than just shorten the code. In this case it makes your code more cohesive. Cohesiveness in code is how well your code works together, and it can often be achieved by making each part of the code perform one specific task like we did above. With the more cohesive code, we have a lot more control over the game loop:

function playNim() {
  var userTurn = true;

  while (true) {
    printBoard();

    if (userTurn) {
      makeMove(getUserMove());
    } else {
      makeMove(getComputerMove());
    }

    if (isGameOver()) {
      break; // break out of game loop
    }

    userTurn = !userTurn;
  }

  printBoard();

  if (userTurn) {
    console.log("You Win!");
  } else {
    console.log("Computer Wins...");
  }
}

We can easily change who goes first. We can easily change the behavior of the computer in getComputerMove. We can easily change how the user inputs their move in getUserMove.


I made a JSFiddle with the all the changes I recommended if you'd like to see it all working together. Keep up the good work!

share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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