Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I am importing an excel document into my project.

I then use the following code to verify that columns have data and display the appropriate error message.

My code is working but it just doesn't look tidy. Is there anyway to refactor all the if statements into a loop to display the error message, possibly another method which checks all?? checkIfColumnIsEmpty is a bool method which returns true if the column is empty

    //if either column 0 and 1 are empty && column 2,3,4 and 5 are not
if (!checkIfColumnisEmpty(r.ItemArray[0]) || !checkIfColumnisEmpty(r.ItemArray[1])
&& checkIfColumnisEmpty(r.ItemArray[2]) && checkIfColumnisEmpty(r.ItemArray[3])
&& checkIfColumnisEmpty(r.ItemArray[4]) && checkIfColumnisEmpty(r.ItemArray[5]))
{
    if (checkIfColumnisEmpty(r.ItemArray[0]) && !checkIfColumnisEmpty(r.ItemArray[1]))
    {
        throw new ImportBOQException("Error importing document: First column is empty");
    }
    else if (!checkIfColumnisEmpty(r.ItemArray[0]) && checkIfColumnisEmpty(r.ItemArray[1]))
    {
        throw new ImportBOQException("Error importing document: Second column is empty");
    }    
    else if (!checkIfColumnisEmpty(r.ItemArray[0]) && !checkIfColumnisEmpty(r.ItemArray[1]))
    {
        //all columns are valid so...
        Column0inSpreadsheet = r.ItemArray[0] as string;
        Column1inSpreadsheet = r.ItemArray[1] as string;

        //Other code which performs other operations, once the level as reached this far
    }
}

//if column 0 and 1 are NOT empty && Either column 2,3,4 or 5 is empty
else if (checkIfColumnisEmpty(r.ItemArray[0]) && checkIfColumnisEmpty(r.ItemArray[1])
|| !checkIfColumnisEmpty(r.ItemArray[2]) || !checkIfColumnisEmpty(r.ItemArray[3])
|| !checkIfColumnisEmpty(r.ItemArray[4]) || !checkIfColumnisEmpty(r.ItemArray[5]))
{
    if (checkIfColumnisEmpty(r.ItemArray[2]))
    {
        throw new ImportBOQException("Error importing document: Third column is empty");
    }
    else if (checkIfColumnisEmpty(r.ItemArray[3]))
    {
        throw new ImportBOQException("Error importing document: Fourth column is empty");
    }
    else if (checkIfColumnisEmpty(r.ItemArray[4]))
    {
        throw new ImportBOQException("Error importing document: Fifth column is empty");
    }
    else if (checkIfColumnisEmpty(r.ItemArray[5]))
    {
        throw new ImportBOQException("Error importing document: Sixth column is empty");
    }
    else
//all columns are valid so...
    { 
        Column2inSpreadsheet = (r.ItemArray[2]) as string;
        Column3inSpreadsheet = (r.ItemArray[3]) as string;
        Column4inSpreadsheet = (r.ItemArray[4]) as string;
        Column5inSpreadsheet = (r.ItemArray[5]) as string;

//Other code which performs other operations, once the level as reached this far
    }
}
else
//other errors ot related to empty colums
{
    throw new Exception("Error Uploading");
}

}

share|improve this question

1 Answer 1

up vote 4 down vote accepted

First of all, better names than columnX would be nice. I guess the indentation is a copying-issue, otherwise this would be the next optimization. Furthermore I have the feeling that mixing the && and the || in your condition without parenthesis is not what you want to do?

col0empty=checkIfColumnisEmpty(r.ItemArray[0])
col1empty=checkIfColumnisEmpty(r.ItemArray[1])
col2empty=checkIfColumnisEmpty(r.ItemArray[2])
col3empty=checkIfColumnisEmpty(r.ItemArray[3])
col4empty=checkIfColumnisEmpty(r.ItemArray[4])
col5empty=checkIfColumnisEmpty(r.ItemArray[5])

if ((!col0empty || !col1empty) && col2empty && col3empty && col4empty && col5empty)
{
    if (col0empty && !col1empty) errorWithEmptyColumn("First");
    else if (!col0empty && col1empty) errorWithEmptyColumn("Second");
    else if (!col0empty && !col1empty)
    {
        //Code after validation
    }
}
else if ((col0empty && col1empty) || !col2empty || !col3empty || !col4empty || !col5empty)
{
    if (col2empty) errorWithEmptyColumn("Third");
    else if (col3empty) errorWithEmptyColumn("Fourth");
    else if (col4empty) errorWithEmptyColumn("Fifth");
    else if (col5empty) errorWithEmptyColumn("Sixth");
    else
    {
        //Code after validation
    }
}
else
{
    throw new ImportBOQException("Error Uploading"); //don't throw raw exceptions
}
// } one to much

private void errorWithEmptyColumn(String columnName) throws ImportBOQException
{
    throw new ImportBOQException("Error importing document: "+columnName+" column is empty");
}

For further optimization it is import to know how similar the code in the two remaining branches is?

You could also extract the condition in an variable and give them a name: i.e. rowTypeX and rowTypeY. Maybe the result is more intuitive if you move the handling of the row types in a method each:

empty=col0empty && col1empty && col2empty && col3empty && col4empty && col5empty
rowTypeX=col2empty && col3empty && col4empty && col5empty;
rowTypeY=col0empty && col1empty;
if ( empty) throw new ImportBOQException("Empty row");
else if (rowTypeX) handleRowTypeX(r);
else if (rowTypeY) handleRowTypeY(r);
else throw new ImportBOQException("Error Uploading"); 
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.