views:

153

answers:

11

Possible Duplicate:
Return 'null' or throw exception

Say I have a method

public Person LoadPersonFromDatastore(int personId)
{
  ...
}

What should I do if the person was not found? Is it best to return null or should I throw an Exception? My opinion would be not to use an exception here since it adds no useful information in contrast to returing null.

If exception should be the right thing which type of Exception would you suggest? The closest thing in .NET framework is RecordNotFoundException. Or should I define my own exceptions for that?

+4  A: 

This depends on the internal logic of your domain / app. In brief, if it is normal that a person is not found, and the app is supposed to deal with null Person references properly, you can return null. If, however, it is an abnormal condition, it is preferred to return an exception.

The case with (checked) exceptions is that they force the caller to deal with the issue right away (either at the place of the call, or on a higher level), whereas a returned null value can go unnoticed for a long time, and then cause a null pointer exception in a far away part of the app, where it may already be difficult to track down where it originated from. And the other benefit of using exception is that the normal flow of events is clearly separated from the error handling code.

You can define your own exception, if you plan to use it in multiple places. For one-off events, it may be overkill.

Péter Török
+3  A: 

http://stackoverflow.com/questions/175532/return-null-or-throw-exception

Alberto Zaccagni
It's still a mystery to me that answers with nothing but a plain link to another question in SO receive upvotes. To me, this a comment or even just a close vote.
Andreas_D
Yeah, I'd agree that a short summary would be nice, if posted as an answer.
Franci Penov
A summary would be merely a copy/paste from that answer, adding nothing but incompleteness to the said answer.
Alberto Zaccagni
@Alberto - but that's why we can comment the question. Anyway, I didn't downvote, it's just my personal opinion.
Andreas_D
@Andreas: Yes, of course this could have been a comment, but it's really the answer to the question. :P Edit: No problem with the downvote, whoever made did it, that's understandable ;)
Alberto Zaccagni
One good reason to add some text is for search engines.
JeremyP
But, yes, the linked answer is the right answer to this question.
JeremyP
+1  A: 

Bit of a subjective question, it's really a matter of preference.

Personally, I'd return null as it's just as easy to handle a null value as it is to handle an exception but with the following benefits:

  • it's more efficient to not generate an exception
  • you don't have to add exception to your method signature
  • you don't have to confuse the code with try catch blocks etc

unless of course, not finding the entity is indeed an error in which case it's better to throw an exception.

pstanton
A: 

It totally depend on what your app supposed to do, if person's info is not in DB. Handling the null is a good idea i guess.

Dheeraj Joshi
Returning `null` is a common cause for unexpected NullPointerExceptions. Try to avoid `null` results in your own API, usually there's always an alternative.
Andreas_D
But it is better than trowing exception i guess.
Dheeraj Joshi
@Dheeraj - to be more precise: there's always an alternative *apart from* throwing an exception. Look at my answer, I provided two ways to avoid 'null' results.
Andreas_D
Point taken. But personally i do not want to put logic in catch block.(And i guess many people think the same). Clearly if we throw some exception, are you suggesting OP to check which instance that exception belong and if it is null pointer exception then consider the id is null?
Dheeraj Joshi
A: 

Sounds like a good candidate for the TryXXX pattern if you expect the failure to happen in common cases:

bool TryLoadPerson(int personId, out Person result);

Edit: As MSDN suggests the TryXXX pattern should be used if the function is expected to fail in common scenarios, otherwise exception is the correct way to communicate a failure. It clearly communicates this intent to the caller as opposed to XXX function which returns "null" for failure. This is part of .NET Framework design guidelines, so every developer that understands them should instantly understand the intent without resorting to reading the documentation of the particular function.

Filip Navara
This is exactly like returning null, only worse. Why should anyone do it this way? I understand that it is done in TryParse() since integers cannot be null (Nullable types where unknown at this time I suggest)
codymanix
agree with codymanix - where's the point?
krtek
@codemanix: It clearly marks the intent of the returned value. The semantics of TryXXX methods are clear on the fact how the failure is communicated to the caller without having to dive into the documentation of the particular function.Also it allows to easily write code in the form of if (a.TryLoadPerson(1234, out person)) { ... }. If "null" was returned then you'd get person = a.LoadPerson(1234); if (person != null) { ... } or worse if ((person = a.LoadPerson(1234)) != null) { ... }
Filip Navara
This is a technical pattern to avoid performance problems related to exceptions. So in fact this is a replacement for throwing an exception in special performance critical parts of the code. Valid, but I wouldn't use it in this case.
Andreas_D
@Andreas_D - If the function is expected to return a valid object in the common scenario then I would go with exception and agree completely with you.
Filip Navara
@Filip Navara - the MSDN article is explicit that TryXxx should be used when the call routinely fails. The key word here is "fails". If the record is missing, that still might not be a "failure", but a "success", if the database design allows it.
Franci Penov
+1  A: 

Common recommendations go against returning null values but I agree, that an exception is not correct in this case, because not finding a person in a database is not too exceptional. (see edit below)

Sometimes I define a special object to indicate, that a query didn't lead to a result:

 public final static Person NOT_FOUND = new Person();

and return it in case there's no match:

 public Person LoadPersonFromDatastore(int personId)
 {
    // ... 
    if (notFound) return Person.NOT_FOUND;
 }

Sometimes I wrap the result in a List and returning an empty list is the signal that the person was not found:

 public List<Person> LoadPersonFromDatastore(int personId) {
   Person result = // try to get it somehow
   if (result == null) {
     return Collections.emptyList();
   }else{
     return Collections.singletonList(result);
   }
 }

but of course this would require a change of the method signature.


Edit - there's a valuable comment: it may be exceptional if you have a valid key and expect to get a Person record from the database. In this case: consider throwing a runtime exception, because this state may be the result of an inconsistent and thus unreliable database, so it might be better to stop the application at this point at correct code or data

Edit 2 - If you decide to throw an exception (because you think this is an exceptional state), consider throwing a runtime exception. Because otherwise you just delegate the problem with null values to the calling method:

Person person = null;
try {
   person = LoadPersonFromDatastore(12345);
} catch (PersonNotFoundException e) {
   // handle exceptional case
}

// now person is either an instance of person or null - which
// is pretty similiar to returning null (what I would avoid in any case)
Andreas_D
I would argue that not finding an entity when you already have received the ID from somewhere IS exceptional. Also, returning a list when looking up an ID is confusing and makes the normal case of finding the person much harder to handle.
Thomas Lötzer
@Thomas - fully agree. **If** it is exceptional, then throw an exception. if not, then return someting other than `null`. Just to prevent from bugfixing NPE's on and on. I favour my first suggestion anyway.
Andreas_D
@Andreas_D - an "inconsistent" database does always equate to an "unreliable" database. It is possible that the database is distributed and does optimized partial replication, so the record is non-existing in the current replica. In which case, the database layer might chiose to return null, and schedule the record for explicit fetch later, if the data is not deemed mission critical for the application. Of course, such scenarios are not the most common database use case. :-)
Franci Penov
@Franci - but then: finding no Person for a given id may be expected again... I don't know. At least I wouldn't throw a *checked* exception - see my second edit.
Andreas_D
This kind of argument is exactly why I believe that the name "exception" is a red herring and the decision of where to use exceptions should be based on the features they offer rather than semantic hair-splittting.
Michael Borgwardt
A: 

It's all in the name, really. Is not finding the person record an exceptional situation, or a normal situation? In a real life example, an attempt to enter a secured building with a badge for a person not in the database, would probably result in security guards jumping from black choppers with guns blazing, but asking on the hotel reception for Mr. Jones room might result in a shrug and "Sorry, no such guest at the hotel" response.

Franci Penov
Personally, I think that It's very much NOT in the name and statements like yours quite misguided. Exceptions are just another method of flow control with certain advantages and disadvantages, which have nothing to do with hair-splitting discussions about what is and is not an exceptional situation.
Michael Borgwardt
@Michael Borgwardt - of course, it's a matter of personal opinion. But while you might not agree that exception usage should be limited, you can't argue that exceptions do have higher cost. The bad part about this is that while the choice to incur that cost is made by the code that throws the exception, the price is actually paid by the code that calls it, which leads to overuse of exceptions in API design without real consideration on the performance of the API consumer.
Franci Penov
Meanwhile, the same applies to the use of exceptions as a flow control - the choice whether the exception is used as a flow control is made by the API, but the price is paid by the consumer code, which now has to deal with the equivalent of a "goto" with an unpredictable target. This leads to consumer code that needs to be overprotective and write separate catch handler for each possible "normal" exception. Not to mention that such catch handlers are not easily integrated within the normal flow control structures. (an ugly example - a break; inside a catch() inside a while{})
Franci Penov
+1  A: 

There is a third way which is to change the signature of the method to not return the object itself, but a wrapper around it. The wrapper contains the value (if there is one), a flag property to indicate whether there is a value (e.g. HasValue) and a method or property for retrieval of the value (which throws if there is none). In short, it provides a type-safe way of representing null.

This is the preferred approach in some of the more functional languages, see the Option type in Scala and Maybe in Haskell, for example.

// example C# using a theoretical Option class    
Option<Person> optionallyBob = LookupPerson("Bob");
if (optionallyBob == Option<Person>.None) ... // handle no value
if (!optionallyBob.HasValue) ... // alternative way of handling no value

Bob bob = optionallyBob.Value; // will throw, unless we have already checked there is a value

public Option<Person> LookupPerson(string name)
{
    if (couldNotFindInDatabaseEtc) return Option<Person>.None;

    return Option.Some(new Person("name"));
}

The advantages to this approach are:

  1. The caller is forced to deal with the return value as they are not returned the desired type directly, instead they are returned Option<WhatWasWanted> instead, so they cannot blindly pass it on to a method that expects a WhatWasWanted.
  2. The caller can opt not to check whether the Option contains a value and know that they will get an exception if it is, indeed, empty. This gives the caller control over whether they get an exception or not without adding kludges to the API (such as a flag for whether to throw or Try overloads that don't throw).

I have a class, Box which I use on my projects for this purpose, the code is below which you may freely use. (You may want to consider renaming it: I have some reservations about the name due to the use of 'box' already in primitve value object wrapping, although it is the same premise. Also, I have been thinking it would be better of as a struct as, as a class, one can assign a null to a Box reference!)

Edit: I've taken this opportunity to revist this class and have made some a bit closer to the Option type in Scala. By having the separate Some and None types, it also means that the empty check can be avoided in the call to Value and that the types can be made structs (before it had to be a class to allow mulitple constructors).

/// <summary>
/// Option helper class.
/// </summary>
public static class Option
{
    public static Option<TItem> Some<TItem>(TItem item)
    {
        return new Some<TItem>(item);
    }

    public static Option<TItem> None<TItem>()
    {
        return new None<TItem>();
    }
}

/// <summary>
/// An optional value.
/// </summary>
/// <typeparam name="TItem"></typeparam>
public interface Option<out TItem>
{
    /// <summary>
    /// Whether the optional value actually has a value.
    /// </summary>
    bool HasValue { get; }

    /// <summary>
    /// The value.
    /// </summary>
    /// <exception cref="InvalidOperationException">The optional value has no value.</exception>
    TItem Value { get; }
}

/// <summary>
/// Some value: an optional value that actually has a value.
/// </summary>
/// <typeparam name="TItem"></typeparam>
public struct Some<TItem> : Option<TItem>, IEquatable<Some<TItem>>, IEquatable<None<TItem>>
{
    /// <summary>
    /// Constructs a new optional value with value.
    /// </summary>
    /// <param name="item">The value.</param>
    public Some(TItem item)
    {
        this.item = item;
    }

    /// <summary>
    /// Whether the optional value actually has a value.
    /// 
    /// Returns true always for Some.
    /// </summary>
    public bool HasValue
    {
        get { return true; }
    }

    /// <summary>
    /// The value.
    /// 
    /// Does not throw.
    /// </summary>
    public TItem Value
    {
        get { return this.item; }
    }

    public bool Equals(Some<TItem> other)
    {
        return Equals(other.item, this.item);
    }

    public bool Equals(None<TItem> other)
    {
        return false;
    }

    public override bool Equals(object obj)
    {
        if (ReferenceEquals(null, obj)) return false;
        if (obj.GetType() != typeof (Some<TItem>)) return false;

        return Equals((Some<TItem>) obj);
    }

    public override int GetHashCode()
    {
        return this.item.GetHashCode();
    }

    public static bool operator ==(Some<TItem> left, Some<TItem> right)
    {
        return left.Equals(right);
    }

    public static bool operator ==(Some<TItem> left, None<TItem> right)
    {
        return false;
    }

    public static bool operator !=(Some<TItem> left, Some<TItem> right)
    {
        return !left.Equals(right);
    }

    public static bool operator !=(Some<TItem> left, None<TItem> right)
    {
        return true;
    }

    private readonly TItem item;
}

/// <summary>
/// No value: an optional value that has no value.
/// </summary>
/// <typeparam name="TItem"></typeparam>
public struct None<TItem> : Option<TItem>, IEquatable<None<TItem>>, IEquatable<Some<TItem>>
{
    /// <summary>
    /// Whether the optional value actually has a value.
    /// 
    /// Returns false always for none.
    /// </summary>
    public bool HasValue
    {
        get { return false; }
    }

    /// <summary>
    /// The value.
    /// 
    /// Throws always.
    /// </summary>
    public TItem Value
    {
        get { throw new InvalidOperationException("No value to retrieve."); }
    }

    public bool Equals(None<TItem> other)
    {
        return true;
    }

    public bool Equals(Some<TItem> other)
    {
        return false;
    }

    public override bool Equals(object obj)
    {
        if (ReferenceEquals(null, obj)) return false;
        if (obj.GetType() != typeof (None<TItem>)) return false;

        return Equals((None<TItem>) obj);
    }

    public override int GetHashCode()
    {
        return 0;
    }

    public static bool operator ==(None<TItem> left, None<TItem> right)
    {
        return left.Equals(right);
    }

    public static bool operator ==(None<TItem> left, Some<TItem> right)
    {
        return false;
    }

    public static bool operator !=(None<TItem> left, None<TItem> right)
    {
        return !left.Equals(right);
    }

    public static bool operator !=(None<TItem> left, Some<TItem> right)
    {
        return true;
    }
}
Paul Ruane
In short, you delegate the option whether to let an exception beeing thrown or checking for valid value to the caller. Very interesting idea but I don't can imagine a situation where I would use this kind of pattern.
codymanix
Paul Ruane
A: 

It really depends on how the condition "person not found" is supposed to be handled. The advantage of exceptions is that they can be caught further up the call stack, so the code that calls the method does not have to deal with the condition directly. OTOH, high-level catch blocks can usually only do generic things like logging and cleanup, and catching exceptions close to where they occur is uglier than testing a condition.

Thus:

  • If code that calls the method would usually execute some specific logic to handle the case "person not found" (e.g. create a dummy person with the given id), then returning null is better.
  • If the code that calls the method can't do anything useful about it and would just do some generic error handling, then using an exception that is caught at a higher level is better.

Specifically, this:

Person p = LoadPersonFromDatastore(currentId)
if(p==null){
    p = createDummy(currentId);
}

is better than this:

Person p;
try{
    p = LoadPersonFromDatastore(currentId)
} catch (PersonNotFoundException) {
    p = createDummy(currentId);
}

But this:

public void foo(int currentId) throws PersonNotFoundException{
    Person p = LoadPersonFromDatastore(currentId)
    // process p
}

is better than this:

public boolean foo(int currentId){
    Person p = LoadPersonFromDatastore(currentId)
    if(p==null){
        return false;
    }
    // process p
    return true;
}
Michael Borgwardt
A: 

Personally speaking, I would return null.

  • If you couldn't find the person, it's not exception, you just couldn't find him.

  • If something abnormal during your finding process, exception should be thrown. That's more reasonable.

卢声远 Shengyuan Lu
A: 

I like the Maybe<T> monad (Nullable<T> for any type, not just value type) as a way to indicate if null is a valid value.

public Maybe<Person> LoadPersonFromDatastore(int personId)
{
  ...
}

But another option is to use an Action<T> parameter to that only executes when a valid value is found.

So, in this case:

public void LoadPersonFromDatastore(int personId, Action<Person> onLoaded)
{
  ...
}

Or even better:

public void LoadPersonFromDatastore(int personId, Action<Person> onLoaded, Action onNotFound)
{
  ...
}
Enigmativity