Best practice for matrix operation code

Question

Below is a function that was included in the class I am using to perform operations on matrices. The function returns a boolean value, the first IF statement if NOT TRUE, it returns a value of false, so I assume once it returns this value the rest of the function doesn't execute because it has already returned a value.

If I had written the function I would have included the FOR loop inside an ELSE construct appended to the original IF statement. I'm not a professional programmer so I apologize if the answer is obvious, I just would not have written the code this way and I'm curious if changing all the functions written this way is worth it.

public bool IsIdentityMatrix()
{
    if (!this.IsSquareMatrix())
    {
        return false;
    }

    for (int i = 0; i < this.RowCount; i++)
    {
        for (int j = 0; j < this.ColumnCount; j++)
        {
            decimal checkValue = 0;
            if (i == j)
            {
                checkValue = 1;
            }

            if (mInnerMatrix[i, j] != checkValue)
            {
                return false;
            }
        }
    }
}

See ['Should a function have only one return statement?'][1] - It provides a good rule of thumb. [1]: http://stackoverflow.com/a/733858/2231703 — HikeMike, Apr 01 '13 at 12:22
@Soner: a moderator has left me a note rejecting a move to _Code Review_. No reason was given. — halfer, Apr 02 '13 at 22:19

score 2 · Answer 1 · answered Apr 01 '13 at 11:01

"Early return" logic like this is very common, and it is normal to not indent the rest of the method in a "else". This does tend to only apply to "early returns" which should be all at the top of the method one after another.

Strictly speaking, the method could be split into two and reorganized slightly to make it more readable (that's a subjective statement of course!):

public bool IsIdentityMatrix()
{
    if (IsSquareMatrix())
        return checkSquareMatrixForIdentity();
    else
        return false;
}

private bool checkSquareMatrixForIdentity()
{
    for (int i = 0; i < this.RowCount; i++)
    {
        for (int j = 0; j < this.ColumnCount; j++)
        {
            decimal checkValue = 0;
            if (i == j)
            {
                checkValue = 1;
            }

            if (mInnerMatrix[i, j] != checkValue)
            {
                return false;
            }
        }
    }

    return true;
}

score 1 · Answer 2 · answered Apr 01 '13 at 10:54

You are right of course, the return statement stops execution of the function at that point and the for loop will not be executed.

I wouldn't consider it bad style to not use an else statement but it might have made more clear that the loop will only be executed if the condition fails.

What I like about the code is that it separates the break logic from. Technically an if(this.IsSquareMatrix()){ <loop> }; return false; would have been equivalent but since this is a recursive function, the break condition is clearly denoted, plus there's one indentation level less :)

score 0 · Answer 3 · answered Apr 01 '13 at 10:56

so I assume once it returns this value the rest of the function doesn't execute because it has already returned a value

That is correct. In fact when you use IDE productivity add-ins like Resharper it will prompt you to structure your code in this way to prevent excessive nesting (which can lead to code smell).

Best practice for matrix operation code

3 Answers3