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

Checking winning conditions in a Connect Four game

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
connectfourcheckingwinninggameconditions

Problem

I'm trying to refactor this C# code that uses one true brace style (it's enforced). I'm coming to C# from C++, in the former I could've made x and y to become i and j with reference assignment, but in C# it's not possible.

Should I take the (i_IsRow) condition to a method? it will have 5 parameters and 2 of them will be out parameters, this is definitely code smell...

private int checkRowsOrColsForWin(bool i_IsRow)
{
    int signCounter = 0;
    int x, y;

    for (int i = 0; i < m_Cols; i++)
    {
        for (int j = 0; j < m_Rows; j++)
        {
            if (i_IsRow)
            {
                x = i;
                y = j;
            }
            else
            {
                x = j;
                y = i;
            }

            signCounterLogic(m_Board[x, y], ref signCounter);

            if (Math.Abs(signCounter) == k_FourInARow)
            {
                break;
            }
        }
        if (Math.Abs(signCounter) == k_FourInARow)
        {
            break;
        }
    }
    return signCounter;
}

Solution

-
Don't use Hungarian notation in C#.

-
The existence of x and y seems pointless. This code:

if (i_IsRow)
{
    x = i;
    y = j;
}
else
{
    x = j;
    y = i;
}
signCounterLogic(m_Board[x, y], ref signCounter);


could just as easily be

if (i_IsRow)
{
    signCounterLogic(m_Board[i, j], ref signCounter);
}
else
{
    signCounterLogic(m_Board[j, i], ref signCounter);
}


-

Should I take the (i_IsRow) condition to a method? it will have 5 parameters and 2 of them will be out parameters, this is definitely code smell...

Agreed (though only three parameters if you get rid of x and y), I don't think that warrants a method. The fact that a method (signCounterLogic) encapsulating the actual work done is called in the loop is sufficient I think.

-
Method names should use PascalCase.

-
I don't like an int return value for a method called checkRowsOrColsForWin. What does an int value mean? If it only checks whether it won or not then it should return a bool. To me with its current name it should return void. If it is actually returning specific information about the win then the name should be changed to reflect what it checks for and returns.

-
I don't like the double check for signCounter == k_FourInARow. If the first one is true, the loop breaks, and the same condition is immediately tested again in the outer loop. Unfortunately I've had too much to drink to be able to think of a good way around that.

-
I don't know what signCounterLogic() does with signCounter, but if it only sets the value then I would prefer that the method returns a value rather than having to pass in a variable by ref. Makes for fewer parameters and a more normal looking method.

-
Assuming the "sign" in signCounterLogic() isn't a verb, this method should definitely be renamed to say what it's doing.

Code Snippets

if (i_IsRow)
{
    x = i;
    y = j;
}
else
{
    x = j;
    y = i;
}
signCounterLogic(m_Board[x, y], ref signCounter);
if (i_IsRow)
{
    signCounterLogic(m_Board[i, j], ref signCounter);
}
else
{
    signCounterLogic(m_Board[j, i], ref signCounter);
}

Context

StackExchange Code Review Q#125962, answer score: 4

Revisions (0)

No revisions yet.