HiveBrain v1.2.0
Get Started
← Back to all entries
patterncsharpMinor

Null replacement - Is this LINQ readable?

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
thislinqreadablenullreplacement

Problem

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().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?

Solution

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

Code Snippets

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 ?? "";
}
foreach (DataGridViewRow row in dGV_factory.Rows)
{
      foreach (DataGridViewCell cell in row.Cells)
      {
          cell.Value = cell.Value ?? "";
      }
}

Context

StackExchange Code Review Q#31344, answer score: 9

Revisions (0)

No revisions yet.