4
\$\begingroup\$

Imagine a game of Checkers, but you can move left right up down instead of diagonally. I have made an array to store liberties (available moves) and use if statements to update if a checker can move there.

Checkers can share liberties:

enter image description here

So the little black squares are liberties.

Here's my ugly if statement to update:

function updateLiberties(x, y, color)
{
    liberties[x][y] = 'None';

    if (color == 'white')
    {
        if (liberties[x + 1][y] == 'clear')
        {
            liberties[x + 1][y] = color;
        }
        else if (liberties[x + 1][y] == 'black')
        {
            liberties[x + 1][y] = 'both';
        }

        if (liberties[x - 1][y] == 'clear')
        {
            liberties[x - 1][y] = color;
        }
        else if (liberties[x - 1][y] == 'black')
        {
            liberties[x - 1][y] = 'both';
        }

        if (liberties[x][y + 1] == 'clear')
        {
            liberties[x][y + 1] = color;
        }
        else if (liberties[x][y + 1] == 'black')
        {
            liberties[x][y + 1] = 'both';
        }

        if (liberties[x][y - 1] == 'clear')
        {
            liberties[x][y - 1] = color;
        }
        else if (liberties[x][y - 1] == 'black')
        {
            liberties[x][y - 1] = 'both';
        }
    }

    if (color == 'black')
    {
        if (liberties[x + 1][y] == 'clear')
        {
            liberties[x + 1][y] = color;
        }
        else if (liberties[x + 1][y] == 'white')
        {
            liberties[x + 1][y] = 'both';
        }

        if (liberties[x - 1][y] == 'clear')
        {
            liberties[x - 1][y] = color;
        }
        else if (liberties[x - 1][y] == 'white')
        {
            liberties[x - 1][y] = 'both';
        }

        if (liberties[x][y + 1] == 'clear')
        {
            liberties[x][y + 1] = color;
        }
        else if (liberties[x][y + 1] == 'white')
        {
            liberties[x][y + 1] = 'both';
        }

        if (liberties[x][y - 1] == 'clear')
        {
            liberties[x][y - 1] = color;
        }
        else if (liberties[x][y - 1] == 'white')
        {
            liberties[x][y - 1] = 'both';
        }
    }
}

So a liberty can either be None (a checker is on it), clear (nothing is on it), black, white or both.

Any chance I make this look a little neater?

\$\endgroup\$
4
  • \$\begingroup\$ How is the liberties array used? Is it for human viewing, or for programmatic consumption? Could you provide an example usage? \$\endgroup\$ Jun 1 '17 at 3:58
  • \$\begingroup\$ At the moment its just used to store the liberties. How this game works is if a players peices are completly surrounded by their opponents, they will lose for the sake of argument. Using an array to store each liberty on the board is one way to check when this happens. I mean it works just fine but just having a big long if statment isnt ideal. It wont even be seen by the human though, right now ive put it on another canvas for visualisation - imgur.com/2XCyu96 . but after this it will just be used programaticly \$\endgroup\$ Jun 1 '17 at 4:03
  • \$\begingroup\$ Is this an infinite board? What happens at the edge? \$\endgroup\$ Jun 1 '17 at 4:12
  • \$\begingroup\$ the board size can vary, normally its 19 x 19. at the edge for example if i place a black peice in the top left corner. it will have 2 liberties, to the right and below. Liberties wont go off the edge \$\endgroup\$ Jun 1 '17 at 4:18
1
\$\begingroup\$

Well, to start with, there is 99% the same code, repeated twice. This is a very bad code smell. You need to turn the repeated code part into a function, like

function checkLiberties (color, otherColor) {
    if (liberties[x + 1][y] == 'clear') {
        liberties[x + 1][y] = color;
    } else if (liberties[x + 1][y] == otherColor) {
        liberties[x + 1][y] = 'both';
    }

    if (liberties[x - 1][y] == 'clear') {
        liberties[x - 1][y] = color;
    } else if (liberties[x - 1][y] == otherColor) {
        liberties[x - 1][y] = 'both';
    }

    if (liberties[x][y + 1] == 'clear') {
        liberties[x][y + 1] = color;
    } else if (liberties[x][y + 1] == otherColor) {
        liberties[x][y + 1] = 'both';
    }

    if (liberties[x][y - 1] == 'clear') {
        liberties[x][y - 1] = color;
    } else if (liberties[x][y - 1] == otherColor) {
        liberties[x][y - 1] = 'both';
    }   
}

and then just call it twice, as

checkLiberties("black", "white");
checkLiberties("white", "black");

After this step, you need to check the same thing inside the function. Where you can spot a pattern in the code, try to turn into a function. I am sure the four chunks inside can become one.

\$\endgroup\$
1
  • \$\begingroup\$ Thanks, i could then shorten it even further by passing in the x and y im checking so in the end its just 1 if statement, called 4 times \$\endgroup\$ Jun 1 '17 at 19:28
1
\$\begingroup\$

It's easy to see that there's a lot of code duplication going on here. What might not be easy to see is a nice way of condensing it. One way to start is to see the similarities and where the lines of code differ. It appears you want to perform an action on each of the four directly neighboring cells and something separate for the center cell. A nested for loop could help tremendously. The only thing to consider here is ignoring the diagonal neighbors.

The other piece of duplication shows up where you do one set of actions when the color argument is one value, and an almost identical set of actions when the argument is the other value. The only place the repeated lines of code differ is at a single hard-coded value. If you just set a variable to be the correct value for either case, you don't have to repeat yourself.

Something you may have not considered is trying to index the array past the boundaries. You should avoid it so you don't encounter errors. Simply add a check for this before indexing the array. I've split this check into a separate function because the boolean expression is quite lengthy on its own, let alone compounded with other boolean expressions.

Another tip is to handle all the cases. If you use a switch, use a default at the end. If you have an else if, use an else at the end. Even if you just want to ignore the case that didn't match the other cases you do handle, you can just write a comment, log a message, or even throw an error.

function updateLiberties(x, y, color) {
    let otherColor;

    // get the other color
    if(color == 'black') {
        otherColor = 'white';
    } else if(color == 'white') {
        otherColor = 'black';
    } else {
        // do nothing for other color values
        return;
    }

    // visit all neighbors
    for(let h = -1; h <= 1; h++) {
        for(let k = -1; k <= 1; k++) {
            // check if diagonal or index is out of bounds
            if(h != 0 && k != 0 || !isIndexValid(x + h, y + k)) {
                // skip, this cell is not direct neighbor or off the edge of the board
                continue;
            }

            // cache value to make shorter lines where this value is repeated
            const liberty = liberties[x + h][y + k];

            if(h == 0 && k == 0) {
                // center cell
                liberties[x + h][y + k] = 'None';
            } else if(liberty == 'clear') {
                liberties[x + h][y + k] = color;
            } else if(liberty == otherColor) {
                liberties[x + h][y + k] = 'both';
            } else {
                // do nothing for other liberty values
                continue;
            }
        }
    }
}

function isIndexValid(x, y) {
    return x >= 0 && x < liberties.length && y >= 0 && y < liberties[x].length;
}
\$\endgroup\$
1
  • \$\begingroup\$ Thankyou, this is abit more complex than i would of liked by it works and alot cleaner than what i had \$\endgroup\$ Jun 1 '17 at 19:29
1
\$\begingroup\$

Updating a Single Liberty

You need a function that does the 'clear'/otherColor update that you do 8 times in your function. Something like updateLiberty so you don't need to copy/paste the if-statement for checking a single liberty when you have 4.

function updateLiberty(liberty, color, otherColor) {

    if (liberty === 'clear') {
        liberty = color;
    } else if (liberty === otherColor) {
        liberty = 'both';
    }

}

Notice the following,

  • I use === instead of ==. Using only two allows for type conversion and may have unintended results if you ever pass it anything other than an expected value. See this answer for more information.

  • The name is updateLiberty and not checkLiberty. While checking the liberty might be a part of what updateLiberty does, using the name checkLiberty completely skips over the fact that updateLiberty changes the value of the liberty. So the best name would probably be checkAndUpdateLiberty but I'll leave that one up to you.

Updating all Liberties

function updateLiberties(x, y, color) {

    var otherColor = color === 'white' ? 'black' 
                   : color === 'black' ? 'white'
                   : color === '';
    liberties[x][y] = 'None';

    updateLiberty(liberties[x + 1][y], color, otherColor);
    updateLiberty(liberties[x - 1][y], color, otherColor);
    updateLiberty(liberties[x][y + 1], color, otherColor);
    updateLiberty(liberties[x][y - 1], color, otherColor);

}

Note the following,

  • Ternary Operator - Instead of using an if-statement to determine what the opposing color is how about we create a variable and use the ternary operator to assign the opposing or otherColor this way it makes it very clear that the value of this variable, otherColor, is based on the value of color and it's all in one line.
  • Clarity of Repetition - Now that we're using the updateLiberty function it should be very clear the part in your code that changed and stayed the same. You needed to be able to execute the same action on each liberty, therefore you can now use the updateLiberty function to update each of the four liberties.
\$\endgroup\$
3
  • \$\begingroup\$ The thing I'm worried about the ternary operator is if the color variable is not always "black" when it isn't "white". In other words, if color can be "clear" or "both", those cases should be ignored, not treated as "black". Otherwise, I'm all for it! Everything else was well thought out and very helpful. \$\endgroup\$
    – kamoroso94
    Jun 1 '17 at 13:07
  • \$\begingroup\$ That is a good point, thanks for the catch. Honestly, if I were the OP I'd create some sort of enumeration-like object to cover all of the possible values, maybe even make some helpful functions to determine the opposite of one of them (i.e. white -> black and vice-versa). \$\endgroup\$
    – Shelby115
    Jun 1 '17 at 14:01
  • \$\begingroup\$ thankyou very much for this info, you put alot of effort i appriciate this alot, ive read it and it makes sense, ive managed to polish everyones suggestions and my own code, ill post what i have now below. i agree with the names though, ill change them \$\endgroup\$ Jun 1 '17 at 19:33

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.