Simplifying a LINQ expression

Question

I have a part of code which I really do not like, if it's possible to simplify it somehow - would be really nice.

A a; // I want to get rid of this variable
if((a = collection.FirstOrDefault(x => x.Field == null)) != null)
{
  throw new ScriptException("{0}", a.y); //I need to access other field of the object here, that's why I had to declare a variable outside of the expression
}

Answer 1

You can make your code much more readable if you will combine variable assignment and definition:

A a = collection.FirstOrDefault(x => x.Field == null);

if(a != null)    
   throw new ScriptException("{0}", a.y);

Answer 2

Rather than finding the first item that matches and handling that, treat the results as a collection. foreach over all of the items that match using Where. Since the exception will bring you back out of the loop the end result is identical, just with cleaner code:

foreach(var a in collection.Where(x => x.Field == null))
    throw new ScriptException("{0}", a.y);

If you want to make it clearer to the reader that the loop will only execute once at most, you can add a Take call in there to clarify the code without making any functional change:

foreach(var a in collection.Where(x => x.Field == null).Take(1))
    throw new ScriptException("{0}", a.y);

This also makes it easier to aggregate all of the invalid items, rather than the first:

var exceptions = collection.Where(a => a.Field == null)
    .Select(a => new ScriptException("{0}", a.y))
    .ToList();
if (exceptions.Any())
    throw new AggregateException(exceptions);

Answer 3

You can't avoid declaring the variable since you need to assign outside of the if and then reference the value inside. (The only way would be to perform the filter twice, which is likely to be expensive).

That said, you can use this to your advantage and make the code more readable:

A a = collection.FirstOrDefault(x => x.Field == null); // assign here
if(a != null) // more easily-readable comparison here
{
  throw new ScriptException("{0}", a.y); 
}

Answer 4

You can't get rid of A a in this situation. You need to store the value returned from your LINQ statement to use later, and, unlike a using block, an if statement doesn't allow you to define a variable in its expression.

Personally, I'd do this:

A a = collection.FirstOrDefault(x => x.Field == null);
if(a != null)
{
    throw new ScriptException("{0}", a.y);
}

Answer 5

So your logic is: If there are any items with no field, throw an exception.

var a = collection.Where(x => x.Field == null);
if(a.Any())
{
  throw new ScriptException("{0}", a.First().y);
}

An even better way might be to collate them:

var a = collection.Where(x => x.Field == null).Select(x => x.y);
if(a.Any())
{
  throw new ScriptException("{0}", string.Join(',', a));
}

That way you can see all of the instances.