Multiple values against same condition

Posted on

Problem

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);
}
}
``````

Solution

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

Posted in C#Tagged