3

I have a samle code which calls SingleOrDefault method 3 times and logs exception if any sequence has more than one matching element.

The problem starts if I want to check which part of this code throws exception.

Is it possible to get some useful information from this exception like predicate parameter or collection type for more detailed trace?

like this - Sequence contains more than one matching element. Collection IEnumrable|ParamType| param {Predicate param toString()}

 public void GetSingleOrDefaultTest(){

    try{

        var user = Users.SingleOrDefault(e => e.Id == 1);

        var profile = UserProfiles.SingleOrDefault(e => e.Id == 1);

        var profile2 = UserProfiles.SingleOrDefault(e => e.Id == 2);


    } catch(InvalidOperationException ex){
        Log(ex);
    }

}
6
  • 1
  • 1
    You're doing it wrong in the first place. Don't rely on exception handling for control flow. You'll accidentally catch true bugs and hide them, among other problems.
    – usr
    May 15, 2014 at 10:32
  • @usr It's not clear to me from this snippet alone that it is being used for control flow May 15, 2014 at 10:43
  • @BenAaronson if exceptions are an expected part of execution (and avoidable), then they are used for control flow. That would be my definition.
    – usr
    May 15, 2014 at 10:48
  • @usr Yeah I'm just not sure that's the case in this situation. Why would multiple users with the same ID be an expected part of execution? This looks like standard "check my data didn't somehow get into a weird unexpected state" code May 15, 2014 at 11:14

5 Answers 5

7

If you want to know which of the statement issues the error you have to check them separateley. Catch the InvalidOperationException on every SingleOrDefault invocation and wrap it in a new exception which you can fill with additional information.

try
{
    User user;
    UserProfile profile;
    UserProfile profile2;

    try
    {
        user = Users.SingleOrDefault(e => e.Id == 1);
    }
    catch (InvalidOperationException ex)
    {
        throw new InvalidOperationException("User lookup for Id = 1 failed", ex);
    }

    try
    {
        profile = UserProfiles.SingleOrDefault(e => e.Id == 1);
    }
    catch (InvalidOperationException ex)
    {
        throw new InvalidOperationException("User profile lookup for Id = 1 failed", ex);
    }

    try
    {
        profile2 = UserProfiles.SingleOrDefault(e => e.Id == 2);
    }
    catch (InvalidOperationException ex)
    {
        throw new InvalidOperationException("User profile lookup for Id = 2 failed", ex);
    }

    // work with user, profile and profile2
}
catch(InvalidOperationException ex)
{
    Log(ex);
}

Edit:

You also can encapsulate the single try catches by the following

private static T GetSingleOrDefault<T>(IEnumerable<T> collection, Expression<Func<T, bool>> predicate)
{
    try
    {
        return collection.SingleOrDefault(predicate.Compile());
    }
    catch (InvalidOperationException e)
    {
        var message = string.Format(
            "{0} (Collection: {1}, param: {2})",
            e.Message,
            collection.GetType(),
            predicate);

        throw new InvalidOperationException(message);
    }
}

so that your code would look like

try
{
    var user = GetSingleOrDefault(Users, e => e.Id == 1);

    var profile = GetSingleOrDefault(UserProfiles, e => e.Id == 1);

    var profile2 = GetSingleOrDefault(UserProfiles, e => e.Id == 2);

    // work with user, profile and profile2
}
catch(InvalidOperationException ex)
{
    Log(ex);
}

This yields in a message like

System.InvalidOperationException: Sequence contains more than one matching element (Collection: IEnumerable`1[User], param: e => e.Id == 1)

1

Whenever you use SingleOrDefault, you clearly state that the query should result in at most a single result. On the other hand, when FirstOrDefault is used, the query can return any amount of results but you state that you only want the first one.

I personally find the semantics very different and using the appropriate one, depending on the expected results, improves readability.

Reference

1
  • This is true, but in this case I think SingleOrDefault is what he actually wants May 15, 2014 at 11:45
1

There is no reasone to use SingelOrDefault. I would refactor this to:

var user = Users.Count(e => e.Id == 1);
var profile = UserProfiles.Count(e => e.Id == 1);
var profile2 = UserProfiles.Count(e => e.Id == 2);

if(user + profile + profile2 != 3){
  Log("more than one");
}

This do basically the same but is not Exception driven. And I don't see in your question a reasone use exception driven programing.

0

That's where logging comes into scene (NLog usage sample, available from NuGet):

public void GetSingleOrDefaultTest()
{
    try
    {
        Logger.Debug("Getting user with id = {0}", 1);
        var user = Users.SingleOrDefault(e => e.Id == 1);

        Logger.Debug("Getting user profile with id = {0}", 1);
        var profile = UserProfiles.SingleOrDefault(e => e.Id == 1);

        Logger.Debug("Getting user profile with id = {0}", 2);
        var profile2 = UserProfiles.SingleOrDefault(e => e.Id == 2);
    } 
    catch(Exception ex)
    {
        Logger.ErrorException("Failed getting single or default", ex);
    }
}

One glance at log file will tell you which statement has failed:

2014-05-26 12:04:48.8655 DEBUG Getting user with id = 1
2014-05-26 12:04:48.8815 DEBUG Getting user profile with id = 1
2014-05-26 12:04:48.8815 DEBUG Getting user profile with id = 2
2014-05-26 12:04:48.8815 ERROR Failed getting single or default
Sequence contains more than one matching element
-2

Before I answer: Please be aware of the perils of using exception handling here. In the comments in this question I have pointed out a few problems. In the end you might worsen your log output by logging misleading information.

Don't rely on exception handling for control flow. You'll accidentally catch true bugs and hide them, among other problems.

Exceptions also have very bad performance.

It is very error prone to use exception handling to diagnose specific issues. Let's see if we can get away without using it.

public static TSource SingleOrDefaultWithDiagnostics<TSource>(
  this IEnumerable<TSource> source,
  Func<TSource, bool> predicate,
  string failureMessage) {

    using (IEnumerator<TSource> enumerator = source.Where(predicate).GetEnumerator())
    {
        if (!enumerator.MoveNext())
        {
            return default(TSource);
        }
        TSource current = enumerator.Current;
        if (!enumerator.MoveNext())
        {
            return current;
        }
        else throw new SingleElementException(failureMessage);
    }
}

I've taken some BCL code and changed the way the error is thrown.

This outputs a meaningful exception in exactly the case that you are looking for. You can attach a custom diagnostic message.

 var user = Users.SingleOrDefaultWithDiagnostics(e => e.Id == 1, "User with Id 1");
 var profile = UserProfiles.SingleOrDefaultWithDiagnostics(e => e.Id == 1, "...");
 var profile2 = UserProfiles.SingleOrDefaultWithDiagnostics(e => e.Id == 2, "...");
5
  • 1
    Could you explain the point of this method? It looks like you've just reimplemented SingleOrDefault May 15, 2014 at 11:28
  • @BenAaronson you can now tell from the exception exactly what happened. That is not the case with the original which throws an undifferentiated IOE.
    – usr
    May 15, 2014 at 11:29
  • 1
    The documentation for SingleOrDefault says it throws an IOE if "source has more than one element." That's the same as yours, except you're using a different exception type. May 15, 2014 at 11:32
  • The predicate could throw as well, or there might be some surrounding code that also can throw.
    – usr
    May 15, 2014 at 11:41
  • But in this case, neither of those are true (yes okay, Id could have some weird get logic, but it almost certainly doesn't). This is very confusing advice to give as an answer to this question. May 15, 2014 at 11:50

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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