12

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.

|improve this question
  • 2
    This is just my person opinion, but I've always found it to make most sense that a method should throw an exception when it does something unintended (exceptional). In your case, I would throw an InvalidOperationException if someone tried to delete null/0 items. – Falgantil May 24 '18 at 7:24
  • 5
    Possible duplicate of I've been told that Exceptions should only be used in exceptional cases. How do I know if my case is exceptional? – gnat May 24 '18 at 7:26
  • 1
    @gnat My question is specifically about services, because of how they are used and their purpose. This "duplicate" only talks about the general cases and the main answer even contradicts itself in the context of my exact method. – FCin May 24 '18 at 8:42
  • 2
    Is it possible for the enumerable to be empty instead of null? Would you handle that differently? – JAD May 24 '18 at 11:03
  • 13
    @Falgantil I can see null being worthy of an exception, but I think it's absurd to throw an exception on an empty list. – Kevin May 24 '18 at 14:57
58

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.

|improve this answer
  • 9
    In any case, if you're going to throw, then there's not all that much utility in throwing AhaINoticedYouPassingInNullException when the runtime already provides you with the facility of throwing NullReferenceException for you, without you writing any code at all. There is some utility (e.g. your code coverage tool will tell you whether or not you have a unit test for passing in null in the former case but not the latter), but neither exception should be try/caught on every call to the service, because it's usually a programmer error. – Steve Jessop May 24 '18 at 14:30
  • 2
    ... you should only be immediately catching exceptions raised due to sketchy parameters, if you know that what you're passing in is null in some expected case, and you actively want the function you're calling to validate it for you. Like Kilian says in the answer, your general policy might be to ban null everywhere it's not explicitly allowed. Then you wouldn't catch NullReferenceException or AhaINoticedYouPassingInNullException anywhere except at high-level disaster-recovery code. And that exception handler should probably create a bug report :-) – Steve Jessop May 24 '18 at 14:35
  • 2
    @FCin: your interface design should match what users actually need out of the service. So if every caller is going to put try/catch around this, then either this method or some convenience method alongside it should check for and ignore null, because that's what your public demands. However, as Kilian says, it usually works out better to treat propagating nulls as a programming error, and not try to continue at every call site. For that matter, what if the service on which this method gets called is null -- do the controllers contain boilerplate to catch and continue in that case too? – Steve Jessop May 24 '18 at 15:01
  • 2
    ... if so, then it does seem like the code base is consistently treating missing components as an expected condition that you should handle at low level and continue. You can reasonably ask, "whose responsibility is it to provide a service and an enumerable so that this operation can go ahead?". If the answer is "someone", then your function is checking preconditions, and the result of a failed precondition normally would be an exception caught at high level, not at every call. If the required stuff for the operation is truly optional, then your function is handling an expected use case. – Steve Jessop May 24 '18 at 15:10
  • 2
    Explicitly throwing an ArgumentNullException is superior to allowing interior code to throw a NullReferenceException -- purely looking at the exception message it is obvious that it is an input error rather than a logic error in the method body at the throw site, and ensuring early-exit means that you are more likely to have left other data in a valid state rather than partially mutated from an unexpected null later on. – Miral May 25 '18 at 2:10
9

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.

|improve this answer
1

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.

|improve this answer
  • I'm in the middle of "shaping" how my services are structured, so some refactoring might be required to throw on invalid input, but not a lot. But I agree with your point that nulls will become an exception once I start throwing instead of hiding them. – FCin May 24 '18 at 8:46
0

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.

|improve this answer
  • "if the code has no reasonable course of action to take in the current context". But my thinking is, it has reasonable course of action, which is to return void. It does exactly the same thing as when passing empty collection, so if I allow empty collection then why would I not allow null. – FCin May 24 '18 at 8:30
  • 1
    @FCin Because of an empty collection you know it is empty. With null you have no collection. If the calling code is supposed/expected to supply a collection to the method, there is something wrong, so you should throw. The only reason to treat a null the same as an empty collection is if you could reasonably expect the calling code to produce null collections. As other answers noted, most of the times, something being null is an anomaly. If you treat these the same as empty collections, you are potentially ignoring a problem elsewhere in the code. – JAD May 24 '18 at 11:18
  • @FCin: also, if you take null to mean empty, that's context-specific to this method. Somewhere else in the same code base, you might have an IEnumerable or IContainer parameter that does some sort of filtering, null might mean "no filter" (allow everything) whereas an empty filter means allow nothing. And in another place, null might explicitly mean "I don't know". So, given that null doesn't always mean quite the same thing everywhere, it's a good idea not to invent any meaning for it at all unless that meaning is necessary or at least useful to someone. – Steve Jessop May 24 '18 at 14:46
0

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.

|improve this answer
  • 1
    "Is it really the job of a library method to enforce rather large policy like null handling?" -- down that line of thinking lie asserts and debug modes, which allow you to help with enforcing a policy, rather than actually enforcing it, if that's what you want to do. So if you're going to swallow null on the principle of being liberal in what you accept, then logging or even throwing would be helpful to people whose intention is to be strict in what they pass in, but who have messed up both their code and their unit tests to the extent that they pass null anyway. – Steve Jessop May 24 '18 at 14:55
  • @SteveJessop, I don't really know if you're arguing against the spirit of my answer, or whether you are continuing along its line. That said, I'm not suggesting to simply swallow null, but to declare it openly in the contract of the method that it is acceptable (or not, depending on whatever solution the OP comes up), and then the question whether to throw an exception (or not) solves itself. – AnoE May 24 '18 at 19:14

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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