CODEWITHSUNDEEP
c#exception
rutruth
rutruth

Reputation: 790

Exceptions - Best Practices

I have been thinking generally about exception handling.

What would be the best practice for implementing a method that gets a User object based on the supplied username parameter. See below. 

    /// <summary>
    /// Gets a user.
    /// </summary>
    /// <param name="username">Username</param>
    /// <returns>User instance</returns>
    public Model.User GetUser(string username)
    {
        return Context.Users.SingleOrDefault(u => u.Username.ToLower() == username.ToLower());
    }

if no user exists with that username parameter, would it be better to return a null User object or rather throw a custom exception specifying that the user does not exist.

Upvotes: 2

Views: 544

Answers (5)

MgSam
MgSam

Reputation: 12803

I disagree with the previously posted answers.

Exceptions should be used only in exceptional circumstances. Failing to find a matching value is not exceptional. IMO, returning null is much more elegant- it leaves the decision about whether to throw an exception to the caller, not the callee, and it makes sense; you ask the method for a user and if there is none it returns nothing.

In addition, your code and the other answers iterate through the entire collection each time, which is going to be unnecessarily slow. You should use a dictionary for quick lookups, (unless, of course, there are frequent changes to the Users collection which makes caching impossible).

Example:

class MyClass
{
    private Dictionary<String,User> _userLookup;

    public MyClass()
    {
        _userLookup = Context.Users.ToDictionary(u => u.UserName.ToLower());
    }

    private User getUserByName(String name)
    {
        var lowerName = name.ToLower();
        return _userLookup.ContainsKey(lowerName) ? _userLookup[lowerName] : null;
    }     
}

Upvotes: 0

weston
weston

Reputation: 54781

First I just want to suggest an improvement on your method of comparing strings without case sensitivity.

/// <summary>
/// Gets a user.
/// </summary>
/// <param name="username">Username</param>
/// <returns>User instance</returns>
public Model.User GetUser(string username)
{
    return Context.Users.SingleOrDefault(u => 
       string.Compare(u.Username, username, true));
}

String.Compare on MSDN

Now my suggestion on this issue

Rather than return null. You may like to return a Null object, using the null object pattern.

public class User
{

  public static readonly User Null = new Null{Username = "Anonymous"};

  ...

}

Then your method becomes:

public Model.User GetUser(string username)
{
    return Context.Users.SingleOrDefault(u => 
       string.Compare(u.Username, username, true)) ?? Model.User.Null;
}

This is useful in situations where null is undesirable. It removes the need to check for null later. If this user object has rights associated for example, you can just make sure the "null user" has no rights.

Upvotes: 1

John Saunders
John Saunders

Reputation: 161773

Throw an exception. Otherwise, your caller, and your caller's caller, and everyone else will need to check for null, or will need to handle an empty collection.

If this is a general-purpose method, meant to be used in a context where the caller knows he needs to check for null, then I'd do this a bit differently. I would have a private method that returns null if there are no users who match. I would add a caller which uses the "try" pattern:

public bool TryGetUser(string username, out Model.User user)

and also one that simply returns the user, but throws an exception if not found

public Model.User GetUser(string username)

Upvotes: 2

Mark Sherretta
Mark Sherretta

Reputation: 10230

I would say that the calling method can decide whether or not the result of no user being found requires the throwing of an exception. If a username is provided, but not found, returning null makes sense to me and letting the calling code decide how it needs to proceed.

Given how generic the method is, why would you throw an exception if a supplied username is not found? It's one thing if a database error occurs etc, but if the SELECT results in no rows, I think it is not exceptional.

Upvotes: 0

Brad
Brad

Reputation: 1529

My advice would be if the program cannot continue without a user then throw an exception. If it is ok that you cannot find a user or that this would be a log-in fail then I would return null. I would only throw an exception if the program could not continue without getting the user and it could not say redirect to log-on again.

Remember throwing an exception is more of an expensive operation that returning null too (I mean nothing noticeable and do think like this (micro-optimization) but exceptions should be used for normal business logic)

Upvotes: 2

Related Questions