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)))

    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);
  • \$\begingroup\$ Is the double if() at the start a typo/copy paste issue? \$\endgroup\$
    – Kyle
    Dec 17, 2013 at 10:48
  • \$\begingroup\$ can you check the updated code now @Kyle? There is no copy paste or typo issues in the updated code. \$\endgroup\$ Dec 17, 2013 at 10:49
  • \$\begingroup\$ Just wondering about the first two if statements (of three), they are identical. There is also one too many close brace (}). \$\endgroup\$
    – Kyle
    Dec 17, 2013 at 10:56
  • \$\begingroup\$ Thanks for your comments @Kyle. I have now removed the condition. \$\endgroup\$ Dec 17, 2013 at 11:01

1 Answer 1


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

  • 3
    \$\begingroup\$ I think that AnyCellsEmpty() would be much simpler if you used LINQ Any(). \$\endgroup\$
    – svick
    Dec 17, 2013 at 16:28
  • \$\begingroup\$ @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) \$\endgroup\$
    – Kyle
    Dec 17, 2013 at 20:47
  • 2
    \$\begingroup\$ 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). \$\endgroup\$
    – svick
    Dec 17, 2013 at 23:59
  • \$\begingroup\$ 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()? \$\endgroup\$
    – Kyle
    Dec 18, 2013 at 0:25
  • 2
    \$\begingroup\$ 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). \$\endgroup\$
    – svick
    Dec 18, 2013 at 1:02

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.