CODEWITHSUNDEEP
c#linq
Anarion
Anarion

Reputation: 2534

Simplifying a LINQ expression

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
}

Upvotes: 6

Views: 3724

Answers (7)

McGarnagle
McGarnagle

Reputation: 102753

You could use a .Select to remove the outer reference to a, although the result is still kind of a mess:

collection.Where(x => x.Field == null)
    .Select(a => string.Format("{0}", a.y))
    .Take(1).ToList().ForEach(msg => 
        throw new ScriptException(msg);
    );

Take(1) returns an empty enumerable if there are no matching items, so that the block in ForEach will run either zero or one time.

Upvotes: 0

user2911903
user2911903

Reputation: 11

Will this work for you? It is ugly, but you can throw the exception within the lambda.

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

Upvotes: 0

Magus
Magus

Reputation: 1312

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.

Upvotes: 1

Servy
Servy

Reputation: 203820

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

Upvotes: 8

Sergey Berezovskiy
Sergey Berezovskiy

Reputation: 236218

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

Upvotes: 7

Dan Puzey
Dan Puzey

Reputation: 34198

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); 
}

Upvotes: 2

Chris Mantle
Chris Mantle

Reputation: 6683

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);
}

Upvotes: 1

Related Questions