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.

Is there any other way to optimize the below code? I feel the below code is huge for the operations it performs.

{
    if ((currentElement == null ||
        (firstGridRow["Low"].ToString() == string.Empty ||
        firstGridRow["High"].ToString() == string.Empty ||
        firstGridRow["Mean"].ToString() == string.Empty ||
        firstGridRow["StdDev"].ToString() == string.Empty)))
    {
        continue;
    }

    double currentLow = Convert.ToDouble(firstGridRow["Low"]);
    double currentHigh = Convert.ToDouble(firstGridRow["High"]);
    double currentMean = Convert.ToDouble(firstGridRow["Mean"]);
    double currentStdDev = Convert.ToDouble(firstGridRow["StdDev"]);
    if (newRow.Length != 0)
    {
        AddColorList(currentElement, opid, currentLow, "Low", newRow, listCollectionLow);
        AddColorList(currentElement, opid, currentHigh, "High", newRow, listCollectionHigh);
        AddColorList(currentElement, opid, currentMean, "Mean", newRow, listCollectionMean);
        AddColorList(currentElement, opid, currentStdDev, "StdDev", newRow, listCollectionStdDev);
    }
}
share|improve this question
    
Is the double if() at the start a typo/copy paste issue? –  Kyle Dec 17 '13 at 10:48
    
can you check the updated code now @Kyle? There is no copy paste or typo issues in the updated code. –  RanjithKumar Govarthanan Dec 17 '13 at 10:49
    
Just wondering about the first two if statements (of three), they are identical. There is also one too many close brace (}). –  Kyle Dec 17 '13 at 10:56
    
Thanks for your comments @Kyle. I have now removed the condition. –  RanjithKumar Govarthanan Dec 17 '13 at 11:01
add comment

2 Answers 2

up vote 10 down vote accepted

So I'll preface this with it's a little difficult without the actual context of the code, and that all suggestions need to be checked to ensure they don't break the existing flow of control.

A few options:

  • Breaking out repetitive operations into reusable methods
  • Wrapper methods (within reason) to apply some common operations and call some other method

With the assumption that these are inside of a loop, and you're not relying on an exception from the Convert.ToDouble() call to break out then you might consider some approaches like follows.

Simplified from provided example. Assuming additional trailing '}' is not significant.

if (currentElement != null
    && !RowHasEmptyCells(firstGridRow, "Low", "High", "Mean", "StdDev")
    && newRow.Lenth != 0)

{
    AddColourListWithSourceValue(firstGridRow, newRow, "Low", currentElement, opid, listCollectionLow);
    AddColourListWithSourceValue(firstGridRow, newRow, "High", currentElement, opid, listCollectionHigh);
    AddColourListWithSourceValue(firstGridRow, newRow, "Mean", currentElement, opid, listCollectionMean);
    AddColourListWithSourceValue(firstGridRow, newRow, "StdDev", currentElement, opid, listCollectionStdDev);
}

Functions attempting to contain some of the previously duplicated logic:

//return true if any of the named cells in the row are 'empty'
bool RowHasEmptyCells(RowType row, params string[] cellNames)
{
    return cellNames.Any(cellName => row[cellName].ToString() == string.Empty);
}

//RowType is the type of your row object (DataGridRow?)
void AddColourListWithSourceValue(RowType sourceRow, RowType newRow, string cellName, object currentElement, object opid, object listCollection)
{
    double currentValue = Convert.ToDouble(sourceRow[cellName]);
    AddColourList(currentElement, opid, currentValue, cellName, newRow, listCollection);
}

For the above double.Parse or .TryParse might be the way to go for a string > double conversion. If using Parse you could remove the empty cells check and just catch a FormatException/ArgumentNullException

This can likely be improved upon, but I've tried to make the flow of the main part a bit easier to follow whilst keeping roughly the same logic.

Edit 1: Sorry - fixed typo. Also AddColourListWithSourceValue() is just a guess at what's going on :)

Edit 2: Quick edit with @svick's correction on logic and suggestion for LINQ to check for empty. Inverted to allow for the positive method prefix 'Has' instead of a 'CellsNotEmpty'. Would welcome thoughts on !IsSomething() vs IsNotSomething()...

Edit 3: != to == in RowHasEmptyCells() as pointed out by @svick

share|improve this answer
2  
I think that AnyCellsEmpty() would be much simpler if you used LINQ Any(). –  svick Dec 17 '13 at 16:28
    
@svick thanks, Can you comment on how you might achieve this assuming something like a DataGridViewCellCollection? I'm assuming you referring to something like return row.Any(c => c.Value.ToString() == string.Empty || c.Value == null); (if row was a IDictionary<string, string> for example) –  Kyle Dec 17 '13 at 20:47
1  
Actually, now I think you got the logic wrong. The main part of the code should execute if all columns are nonempty, which means your AnyCellsEmpty should be renamed and it should contain if (row[cellName].ToString() == string.Empty) return false. Or using LINQ: return cellNames.All(cellName => row[cellName].ToString() != string.Empty). –  svick Dec 17 '13 at 23:59
    
Thanks again @svick. Quick edit, hopefully that captures the original intent. Would be interested to hear any thoughts on the negations in method names? Just reword to use a different adjective, ie: NamedCellsPopulated()? –  Kyle Dec 18 '13 at 0:25
2  
Yeah, I think it's better this way, methods shouldn't be negated by themselves, because you should never have !IsNotSomething(). And the best way to avoid that is to have all names positive. But you didn't apply de Morgan's laws correctly, it should be cellNames.Any(cellName => row[cellName].ToString() == string.Empty). –  svick Dec 18 '13 at 1:02
show 1 more comment

Something like this could work:

        if (newRow.Length != 0 && currentElement != null)
        {
            double currentLow = (firstGridRow["Low"].ToString() == string.Empty ? 0 : Convert.ToDouble(firstGridRow["Low"]);
            double currentHigh = (firstGridRow["High"].ToString() == string.Empty ? 0 : Convert.ToDouble(firstGridRow["High"]);
            double currentMean = (firstGridRow["Mean"].ToString() == string.Empty ? 0 : Convert.ToDouble(firstGridRow["Mean"]);
            double currentStdDev = (firstGridRow["StdDev"].ToString() == string.Empty ? 0 : Convert.ToDouble(firstGridRow["StdDev"]);
            AddColorList(currentElement, opid, currentLow, "Low", newRow, listCollectionLow);
            AddColorList(currentElement, opid, currentHigh, "High", newRow, listCollectionHigh);
            AddColorList(currentElement, opid, currentMean, "Mean", newRow, listCollectionMean);
            AddColorList(currentElement, opid, currentStdDev, "StdDev", newRow, listCollectionStdDev);
        }
share|improve this answer
add comment

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.