S3267 reported for things that it shouldn't and that weren't being reported yesterday

This loop was in the code for a few weeks now and was not being reported as any problem.

foreach (DataColumn column in rawSheetData.Columns)
   var datum = Convert.ToString(row[column.ColumnName]);
   cs.Data.Add(column.ColumnName, datum);

Today I pushed a PR to fix a legitimate critical issue elsewhere in the code, and while I fixed that, sonarcloud started complaining about rawSheetData.Columns in the code above being a code smell and that it should be converted to a “clearer, more concise LINQ expression instead”.

Sonar should not have just started complaining like that about something. Was anything released in the last week that would have caused this to start getting flagged?

Either way this rule evaluation should be reverted, because a for loop does not get any more readable or easier to follow than this. Adding a LINQ expression to project… what exactly… is going to add to the cognitive burden of reading the rest of a loop and its not going to make the code any more concise or clearer.

The suggestion is to change the above code to something like this…

foreach (var columnName in rawSheetData.Columns.Select(cs => cs.ColumnName))
    var datum = Convert.ToString(row[columnName]);
    cs.Data.Add(columnName, datum);

what is going to happen is compiler errors because you cant LINQ a DataColumnCollection (or a DataRowCollection)

I would have to cast it to make this idea work

foreach (var columnName in rawSheetData.Columns.Cast<DataColumn>().Select(dc => dc.ColumnName))
    var datum = Convert.ToString(row[columnName]);
    cs.Data.Add(columnName, datum);

Which nobody should be doing when they have code like the original loop. All that was accomplished by following this sonar rule was a bit of code gymnastics in order to make column.ColumnName into columnName. And it made the intention of the statements unarguably less clear.

EDIT: Looks like it was just made available yesterday…

Its nice to know that not only will the sonar engine surprise me with a failed PR when I didnt even touch the code its complaining about, but also that sonar source takes the time to insult me and call me clumsy as well. After the sarcasm and irony has been properly digested, kindly burninate the ‘clumsy’ tag

Adding to the point of @StingyJack , why is this a major smell? What are the performance implications of using LINQ instead of a foreach (that in the worst case just allocates an iterator)

It is not a performance issue, it is a readability issue. You’re not iterating columns, but the names of the column. However, because Columns does not implement IEnumerable<T> but only IEnumerable, I think it is fair to state that the readability is not per se improved, as you also have to add the .Cast<T>() on top to .Select().

I get that this improve the readability of the code, but it should not be a major smell if the change could degrade performance.

Hi @StingyJack, thanks for your feedback.

This seems to be a false positive, I’ve added an issue for it and you can follow the progress here: Rule S3267: FP, issue reported for DataColumnCollection · Issue #4996 · SonarSource/sonar-dotnet · GitHub

Regarding the issue severity, I agree that this shouldn’t be major, we will reconsider it.

EDIT: with the new version the severity will be changed to Minor and the tag removed from this rule.

Just as an FYI, this shows up in my build pipeline logs also.

Warning S***67: Loop should be simplified by calling Select(dc => dc.ColumnName))

(the *** masking is courtesy of a bug in Azure Devops)

This topic was automatically closed 7 days after the last reply. New replies are no longer allowed.