5
\$\begingroup\$

This is the original version of code written by my coworker that replaces every null cells with an empty string.

for (int i = 0; i < dGV_factory.Rows.Count; i++)
{

       this.dGV_factory["dGV_factory_groupID", i].Value 
             = this.dGV_factory["dGV_factory_groupID", i].Value ?? "";
       this.dGV_factory["dGV_factory_groupID", i].Value
             = this.dGV_factory["dGV_factory_groupID", i].Value ?? "";
       this.dGV_factory["dGV_factory_groupID", i].Value
             = this.dGV_factory["dGV_factory_groupID", i].Value ?? "";
       this.dGV_factory["UE", i].Value
             = this.dGV_factory["UE", i].Value ?? "";
                ...
}

My instant reaction: Ugh.

Being a shameless fanatic of LINQ, I decided to rewrite the code using LINQ.

foreach(DataGridViewRow row in dGV_factory.Rows)
{
      //Replace every null cells with an empty string
      row.Cells.Cast<DataGridViewCell>().ToList().ForEach(cell => cell.Value = cell.Value ?? "");
}

However, I'm wondering if this code is less readable than the above version, even though it is definitely compact.

In my eyes, it's definitely readable as I'm comfortable with LINQ but my coworkers have not even heard of LINQ.

Which is a better change? Should I keep my LINQ version or unroll LINQ to two foreach loops?

\$\endgroup\$
  • \$\begingroup\$ Opinion based close vote?? I'd really appreciate which part sounds very opinionated, and I'd be glad to edit. Really, I'm just looking for a guidance. \$\endgroup\$ – l46kok Sep 16 '13 at 5:20
  • \$\begingroup\$ "Is this a better change?" seems to be it. I'd reword it to ask which follows best coding practice in accordance to this tag. \$\endgroup\$ – Jamal Sep 16 '13 at 5:41
  • \$\begingroup\$ For the record, List.ForEach() is not part of LINQ. In fact, it goes against the whole idea of functional programming, which is what LINQ is based on. (Although a very similar method is part of PLINQ.) \$\endgroup\$ – svick Sep 16 '13 at 9:28
  • 2
    \$\begingroup\$ No one ever agrees with me on this, but I think the most concise LINQ is the best way to go. Not being comfortable with LINQ is not an excuse to write verbose code. \$\endgroup\$ – The Muffin Man Sep 16 '13 at 16:21
  • \$\begingroup\$ I think there is a fundamental problem here. From my painful experience it is best to handle null transparently at the source, so check out DataGridView.DataSourceNullValue. Beware even the potential for inconsistent null/DBNull handling. \$\endgroup\$ – radarbob Sep 17 '13 at 15:10
8
\$\begingroup\$

List.ForEach might have its uses, but I wouldn't use it here. It's not "wrong" per se, but a classic foreach loop

  • is at least as easy to read as your LINQ expression,
  • does not require the explicit Cast<...>(),
  • does not require ToList().

If you put it on one line, it is actually shorter than your LINQ-based solution.

foreach(DataGridViewRow row in dGV_factory.Rows)
{
      //Replace every null cells with an empty string
      foreach (DataGridViewCell cell in row.Cells) cell.Value = cell.Value ?? "";
}

Although, for readability, I'd prefer:

foreach (DataGridViewRow row in dGV_factory.Rows)
{
      foreach (DataGridViewCell cell in row.Cells)
      {
          cell.Value = cell.Value ?? "";
      }
}

(Of course, this is still a great improvement over your co-worker's code.)

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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