Setting output parameter inside LINQ select

Question

Is there a way to reduce the number of lines by setting the output variable inside LINQ select statement? How would you do it?

public void GetSomeValue(int bonusID, out decimal? maxGiving, out int? isActive)
{
 maxGiving = 0;
 isActive = 0;
 try
 {
 var con = new BonusModelDataContext();
 var a = con.Bonus
 .Where(c => c.ID == bonusID)
 .Select(c => new { maxGiving = c.maxGiving, isActive = c.isActive })
 .SingleOrDefault();
 maxGiving = a.maxGiving;
 isActive = a.isActive;
 // return iovation;
 }
 catch(Exception ex)
 {
 //return -1;
 }
}

Answer 1

No, you can't do that, you can't use out parameters in a lambda.

But even if you could, I don't think it would be a good idea to do so, because it would make the code less readable.

There are also some suspicious practices in your code:

1. You're using SingleOrDefault(), but you don't check the result for null. If you don't want to check for null, use Single() which will throw an exception if the query returns an empty collection.
2. You're catching all exceptions. You should catch only the exceptions you know about. And you certainly shouldn't use something like return -1, it's much better to just let the exception bubble up and catch it somewhere up the call stack. Another option might be catching the exception, wrapping it in some more specific exception and throwing that.
3. Why are you setting the output parameters to 0 by default? They are nullable, so setting them to null makes more sense to me.

Answer 2

svick's observations are spot on, however I suggest you go a bit further in your refactoring.

Suggested Refactorings

Naming

• GetSomeValue is not expressive. I changed it to GetBonusDetails, which makes more sense to me, but then again I don't know the rest of your code base - just choose a name that clearly says what the method does.
• con is borderline acceptable... for clarity I renamed it to dataContext.
• the name a doesn't tell us anything about what this variable contains. I renamed it to bonus.
• the name isActive is a lie. This int is pretending to be a bool, probably because it should indeed be boolean. I don't know for sure, since the range of possible values is not apparent in your code.
  • If it only gets assigned the values 0 and 1, change the data type to bool.
  • If it can have other values, rename it to activityStatus or something similar.

(from now on I will use the corrected names in my explanations and code.)

Exception Handling

• I changed SingleOrDefault() to Single() to avoid a possible NullReferenceException.
• I removed your try-catch-statement: swallowing an error can turn into a debugging headache in the future. I believe that in this case you should probably create a custom BonusNotFoundException. You would then catch this exception at a higher level and notify the user.

Output Parameters

I agree that it would make more sense to initialize the output parameters with null since you made the effort of making them Nullable. But if you take a step back, you realize how ugly the use of out parameters is in this situation. So I deleted them and made GetBonusDetails return an instance of a new class designed to hold two pieces of information: maxGiving and isActive. I called it BonusDetails, but I'm sure you can find a much better name for it.

LINQ Query

I rewrote your query as a query statement because I find the syntax less convoluted. However, using a method chain is also all right if it is more readable for you.

End Result

GetBonusDetails method

public BonusDetails GetBonusDetails(int bonusID)
{
 var dataContext = new BonusModelDataContext();
 var bonus = from b in dataContext.Bonus
 where b.ID == bonusID
 select new BonusDetails(b.maxGiving, b.isActive);
 return bonus.Single();
}

BonusDetails class

public class BonusDetails
{
 public int MaxGiving { get; private set; }
 public bool IsActive { get; private set; }
 public BonusDetails(int maxGiving, bool isActive)
 {
 MaxGiving = maxGiving;
 IsActive = isActive;
 }
}

• Claudiu: Very good advices here (but for LINQ I prefer the method chain way:P), but I recommend you to use using statement for dataContext object in var dataContext = new BonusModelDataContext(); (+1)

Answer 3

Working on codesparkle's answer, I think this might be better still:

public BonusDetails GetBonusDetails(int bonusID)
{
 var dataContext = new BonusModelDataContext();
 var bonus = dataContext.Bonus.First(b =>b.ID == bonusID);
 return new BonusDetails(bonus.maxGiving, bonus.isActive);
}

Assuming .ID is unique (if not then .Single(Func<T,bool>) so you get the exception for more than one element). In both cases you should probably catch the internal exceptions and throw your own in its place to hide the implementation details (or specify in the method documentation that it will throw these internal exceptions, but I would think of that as bad practice).

• Adam: nice one, you have my +1
• svick: I would use Single even if ID was unique, because it makes the intent clearer. And it's also safer (what if ID stops being unique?).