4
\$\begingroup\$

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;
}
\$\endgroup\$
4
\$\begingroup\$
  1. Don't use Hungarian notation in C#.

  2. 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);
}
  1. 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.

  2. Method names should use PascalCase.

  3. 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.

  4. 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.

  5. 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.

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

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ Right very nice, I made the method in steps and didn't see that. Where is there hungarian notation? if you mean the prefixes, then they're enforced coding standard. The case of the methods is also enforced, since it's private then it should be camelCase... \$\endgroup\$ – shinzou Apr 17 '16 at 21:39
  • \$\begingroup\$ signCounterLogic() has to know the current value of signCounter and change it accordingly so it must be a ref. And yeah you're right, I didn't think in term of verbs for its name. Thanks for helping out. \$\endgroup\$ – shinzou Apr 17 '16 at 21:40
  • \$\begingroup\$ Er yeah I mean the prefixes (I did say I had too much to drink...). It's nonstandard in C# to use that sort of stuff. And camelCase is nonstandard for methods in C# regardless of whether they're private or not. If that's "enforced" then someone is mixing up their languages. \$\endgroup\$ – 404 Apr 17 '16 at 21:52
  • \$\begingroup\$ Use if (Math.Abs(signCounter) == k_FourInARow) return signCounter; to avoid having to break in both the inner and outer for loops. Alternatively you could use a goto statement, but that's unnecessary. \$\endgroup\$ – 4castle May 3 '16 at 3:36

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.