1

Trying to refactor following code:

var items = Common.Deserialize<DiagnosisItem[]>(path, false);
if (items != null)
    foreach (var item in items)
    {
        var find = _items.Where(o => o.Value.Tooltip == item.Id).FirstOrDefault();
        if (find.Value != null)
        {
            find.Value.Text = item.Text;
            find.Value.Color = (Color)converter.ConvertFromInvariantString(item.Color);
        }
    }

Talking about null checks. Seems C# 6.0 doesn't add anything new for it. Was thinking "Oh, great, we have ?. and ?[] now! Should be cool!" and then like ... hmm, but it does not happens often to have right side expression...

Am I wrong? Can this code become a bit more beautiful?

7
  • 11
    Would this question be better suited to codereview.stackexchange.com? Jul 28, 2015 at 13:36
  • 4
    .Where(o => o.Value.Tooltip == item.Id) would fail if Value was null, so either that is wrong, or the find.Value != null check is not necessary. You do need a find != null check though.
    – poke
    Jul 28, 2015 at 13:36
  • You could add curlies { } to your if block for a start...
    – musefan
    Jul 28, 2015 at 13:37
  • 2
    You are using a lot of imperative statements. If you make this a query a lot of the mess goes away. I still see no use for the ?. operator, though.
    – usr
    Jul 28, 2015 at 13:39
  • A concise way of annotating parameters and return values to indicate that they can't be null together with an automatic check would have been 10x more useful than the ?. operator. Jul 28, 2015 at 13:42

2 Answers 2

4

I usually like to put as much as possible into queries because queries are more composable than statements.

var items = Common.Deserialize<DiagnosisItem[]>(path, false);
var updates =
 from item in items.NullToEmpty()
 join find in _items on o.Value.Tooltip equals item.Id
 where find.Value != null
 select new { find.Value, item };

foreach (var u in updates)
{
    u.Value.Text = u.item.Text;
    u.Value.Color = (Color)converter.ConvertFromInvariantString(u.item.Color);
}

Here, I see no need for the new null operator.

As a nice side-effect the quadratic time complexity of the FirstOrDefault thing is gone.

2
  • Thanks, this looks much prettier, though I am not a big fun of sql-alike syntax at least for now. Where is this NullToEmpty defined? Is it your own extension method or what? Having such basically will eliminate any need of null check before any foreach, could be pretty useful.
    – Sinatr
    Jul 28, 2015 at 13:57
  • Yes, it's mine and I consider it essential. Alternatively: items ?? Enumerable.Empty() or items ?? new Item[0]. Both are not as nice.
    – usr
    Jul 28, 2015 at 14:01
2

C# 6 a side, I personally prefer the following approach whereby you invert your if statements in order to reduce arrow code.

        var items = Common.Deserialize<DiagnosisItem[]>(path, false);

        if (items == null)
            return;

        foreach (var item in items)
        {
            var find = _items.Where(o => o.Value.Tooltip == item.Id).FirstOrDefault();

            if (find.Value == null)
                continue;

            find.Value.Text = item.Text;
            find.Value.Color = (Color)converter.ConvertFromInvariantString(item.Color);
        }

P.S. This would be better suited on the codereview site

4
  • 1
    As @poke mentioned in comment to OP, there should be a check on whether or not find is null before checking find.Value.
    – Rick Davin
    Jul 28, 2015 at 13:55
  • Well, you still have same null checks, only moved code outside of if, making it more plain (which is good, less indentation). But that's only cosmetic change and doesn't answer question. I am not asking for code review, but for null checks (sorry if that is not clear).
    – Sinatr
    Jul 28, 2015 at 13:59
  • @RickDavin yeah you are right. I was just trying to make a point about reducing arrow code and therefore making it slightly more 'beautiful' Jul 28, 2015 at 14:01
  • @Sinatr unless my eyes deceive me, your question explicitly asks 'how can I make this more beautiful?' - which is surely synonymous with a cosmetic change? Jul 28, 2015 at 14:04

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