Should service throw exception or return when no items specified for deletion

Question

I have a piece of code that can be represented as:

public class ItemService {

    public void DeleteItems(IEnumerable<Item> items)
    {
        // Save us from possible NullReferenceException below.
        if(items == null)
            return;

        foreach(var item in items)
        {
            // For the purpose of this example, lets say I have to iterate over them.
            // Go to database and delete them.
        }
    }
}

Now I'm wondering if this is the right approach or should I throw exception. I can avoid exception, because returning would be the same as iterating over an empty collection, meaning, no important code is executed anyway, but on the other hand I'm possibly hiding problems somewhere in the code, because why would anyone want to call DeleteItems with null parameter? This may indicate that there is a problem somewhere else in the code.

This is a problem I usually have with methods in services, because most of them do something and don't return a result, so if someone passes invalid information then there is nothing for the service to do, so it returns.

Answer 1

These are two different questions.

Should you accept null? That depends on your general policy about null in the code base. In my opinion, banning null everywhere except where explicitly documented is a very good practice, but it's even better practice to stick to the convention your code base already has.

Should you accept the empty collection? In my opinion: YES, absolutely. It is much more effort to restrict all callers to non-empty collections than to do the mathematically right thing - even if it surprises some developers who are iffy with the concept of zero.

Answer 2

Null Value

As @KilianFoth already said, stick to your general policy. If that is to treat null as a "shorthand" for an empty list, do it that way.

If you don't have a consistent policy on null values, I'd recommend the following one:

null should be reserved to represent situations that can't be expressed by the "normal" type, e.g. using null to represent "I don't know". And that's a good choice, as everybody trying to carelessly use this value will get an exception, which is the right thing.

Using null as a shorthand for an empty list does not qualify that way, as there already is a perfect representation, being a list with zero elements. And it's a technically bad choice, as it forces every part of your code dealing with lists to check for the valid shorthand null.

Empty List

For a DeleteItems() method, passing an empty list effectively means to do nothing. I'd allow that as an argument, not throwing an exception, just returning quickly.

Of course, the caller could check for zero elements first and skip the DeleteItems() call in that case. If we're talking about a web API, for efficiency reasons the caller should actually do that to avoid unneccessary traffic and round-trip latencies. But I don't think your API should enforce that.

Answer 3

Throw an exception and handle nulls in the calling code.

As a design rule try to avoid null as parameter values. It will reduce NullPointerExceptions in general, as nulls will really be an exception.

Besides that, look at the rest of your code. If this is a common pattern in your project then stay consistent.

Answer 4

Generally speaking, throwing exceptions should be reserved for exceptional situations, i.e. if the code has no reasonable course of action to take in the current context.

You can apply that thought process to this situation. Given that this is a public class with a public method, you have a public API, and in theory no control over what gets passed to it.

Your code has no context about what is calling it (as it shouldnt), and there is nothing you can reasonably do with a null value. This would certainly be a candidate for an ArgumentNullException.

Answer 5

This question seems not to be about exceptions, but about null being a valid argument.

So, first and foremost you have to decide whether null is an allowed value for your the argument of that method. If it is, then you do not need an exception. If it is not, then you do need an exception.

Whether you wish to allow null, or not, is debatable, as shown by lots and lots of Google hits about that. Meaning, you will not get a clear answer, and it is somewhat up to opinion and tradition at the place you are working.

Another area of contention is whether a library function should be really strict about such things, or as lenient as possible, as long as it is not trying to "fix" erroneous parameters. Compare this to the world of network protocols, mail transfer etc. (which are interface contracts, just like methods in programming). There, usually, the policy is that the sender should conform as strictly as possible to the protocol, while the receiver should go out of their way to be able to work with whatever is coming along.

So you have to decide: Is it really the job of a library method to enforce rather large policy like null handling? Especially if your library is used by other people as well (who might have different policies).

I would probably err on the side of allowing null values, definining and documenting their semantics (i.e., null = empty array in this case), and not throw an exception, unless 99% of your other similar code ("library" style code) does it the other way 'round.