How to avoid convoluted logic for custom log messages in code?

Question

I know the title is a little too broad, but I'd like to know how to avoid (if possible) this piece of code I've just coded on a solution of ours.

The problem started when this code resulted in not enough log information:

...
var users = [someRemotingProxy].GetUsers([someCriteria]);
try
{
    var user = users.Single();
}
catch (InvalidOperationException)
{
    logger.WarnFormat("Either there are no users corresponding to the search or there are multiple users matching the same criteria.");
    return;
}
...

We have a business logic in a module of ours that needs there to be a single 'User' that matches some criteria. It turned out that, when problems started showing up, this little 'inconclusive' information was not enough for us to properly know what happened, so I coded this method:

private User GetMappedUser([searchCriteria])
{
    var users = [remotingProxy]
           .GetUsers([searchCriteria])
           .ToList();

    switch (users.Count())
    {
        case 0:
            log.Warn("No user exists with [searchCriteria]");
            return null;

        case 1:
            return users.Single();

        default:
            log.WarnFormat("{0} users [{1}] have been found"
                          users.Count(), 
                          String.Join(", ", users);
            return null;
    }

And then called it from the main code like this:

...
var user = GetMappedUser([searchCriteria]);
if (user == null) return;
...

The first odd thing I see there is the switch statement over the .Count() on the list. This seems very strange at first, but somehow ended up being the cleaner solution. I tried to avoid exceptions here because these conditions are quite normal, and I've heard that it is bad to try and use exceptions to control program flow instead of reporting actual errors. The code was throwing the InvalidOperationException from Single before, so this was more of a refactor on that end.

Is there another approach to this seemingly simple problem? It seems to be kind of a Single Responsibility Principle violation, with the logs in between the code and all that, but I fail to see a decent or elegant way out of it. It's even worse in our case because the same steps are repeated twice, once for the 'User' and then for the 'Device', like this:

1. Get unique user
2. Get unique device of unique user

For both operations, it is important to us to know exactly what happened, what users/devices were returned in case it was not unique, things like that.

Answer 1

How about something like this?

public class UserResult
{
    public string Warning { get; set; }
    public IEnumerable<User> Result { get; set; }
}

public UserResult GetMappedUsers(/* params */) { }

public void Whatever()
{
    var users = GetMappedUsers(/* params */);
    if (!String.IsNullOrEmpty(users.Warning))
        log.Warn(users.Warning);
}

Switch for a List<string> Warnings if required. This treats your GetMappedUsers method more like a service that returns some data and some metadata about the result, which allows you to delegate your logging to the caller - where it belongs - so your data access code can get on with just doing its job.

Although, to be honest, in this scenario I would prefer simply to return a list of user IDs from GetMappedUsers and then use users.Count to evaluate your "cases" in the caller and log as appropriate.

Answer 2

@AntP hit upon the answer I like best. I think the reason you are struggling is that you actually have two problems here. The first is that the code seems to have too much responsibility. Apply this simple test: give this method a simple name that describes everything it does. If your name includes the word "and", it's doing too much. When I apply that test, I might name it "GetUsersByCriteriaAndValidateOnlyOneUserMatches()." So it is doing two things. Split it up into a lookup function that doesn't care how many users are returned, and a separate function that evaluates your business rule regarding "I can handle only one user returned".

You still have your original problem, though, and that is the switch statement seems awkward here. The strategy pattern comes to mind when looking at a switch statement, although pragmatically I'd consider it overkill in this case.

If you want to explore it, though, think about creating a base "UserSearchResponseHandler" class, and three sub classes: NoUsersReturned; MultipleUsersReturned; and OneUserReturned. It would have a factory method that would accept a list of Users and return a UserSearchResponseHandler based on the count of users (encapsulating the logic of the switch inside the factory.) Each handler method would do the right thing: log something appropriate then return null, or return a single user.

The main advantage of the Strategy pattern comes when you have multiple needs for the data it identifies. If you had switch statements buried all over your code that all depended on the count of users found by a search, then it would be very appropriate. The factory can also encapsulate substantially more complex rules, such as "user.count must = 1 AND the user[0].level must = 42 AND it must be a Tuesday in September". You can also get really fancy with a factory and use a registry, allowing for dynamic changes to the logic. Finally, the factory nicely separates the "interpreting" of the business rule from the "handling" of the rule.

But in your case, probably not so much. I'm guessing you likely have only the one occurrence of this rule, it seems pretty static, and it's already appropriately located near the point where you acquired the information it's validating. While I'd still recommend splitting out the search from the response parser, I'd probably just use the switch.

A different way to consider it would be with some Goldilocks tests. If it's truly an error condition, you could even throw:

if (users.count() < 1)
{
    throw TooFewUsersReturnedError;
}

if (users.count() > 1)
{
    throw TooManyUsersReturnedError;
}

return users[0];  // just right