Returning null in a method that accounts for multiple conditions

Question

Consider the following method:

private static String method (String string) {
    if (string.equals("conditionOne")) {
        return value;
    } else if (string.equals("conditionTwo")) {
        return symbol;
    } else {
        return null;
    }
}

Let's say I am checking for two conditions, conditionOne and conditionTwo. Also, assume that some other part of the program ensures that only these two cases will ever happen. Since the method has to return something for all cases to avoid a compiler error, is it okay to return null for the final else block just for syntactical purposes since that part will never execute?

Edit: For clarity, I'd like to mention that the compiler gives me an error ("Expecting return statement") if I don't include that last else block. Other than to returning null (or an empty string, as pointed out by Anthony below), is there another way to write this method so that this does not happen?

Thanks

The people developing stagefright thought the same. Who would pass more imagedata then the size of the image? Your code will not lead to a big exploit, but to a (may) bad user experience when the code crashes — Jonas Wilms, Jul 24 '16 at 21:57

score 1 · Answer 1 · answered Jul 24 '16 at 21:57

1

As you have defined the function as returning a String, it would be more correct to have

if (string.equals("conditionOne")) {
    return value;
} else if (string.equals("conditionTwo")) {
    return symbol;
} else {
    return "";
}

answered Jul 24 '16 at 21:57

Anthony Staunton

115
2
6

This only offloads the problem - yes you avoid `NullPointerException` but now all your code has to check that the result is not empty. This pattern leads to hard-to-maintain code that is difficult to work with and reason about. – dimo414 Jul 24 '16 at 22:26

pez · Answer 2 · 2016-07-25T00:42:38.653

You can write something like this:

if (string.equals("conditionOne")) {
    return value;
} else if (string.equals("conditionTwo")) {
    return symbol;
}
return "";

or like this:

string rval = "";
if (string.equals("conditionOne")) {
    rval = value;
} else if (string.equals("conditionTwo")) {
    rval = symbol;
}
return rval;

or like this

if (string.equals("conditionOne")) {
    return value;
} else if (string.equals("conditionTwo")) {
    return symbol;
}
throw;

edited.

score 1 · Accepted Answer · edited May 23 '17 at 11:51

1

You're describing a very common scenario while programming. You intend for a certain thing to never happen, but the compiler also knows it could happen. The proper way to handle such code paths is to ensure that they are never reached, generally by throwing an AssertionError or a RuntimeException such as IllegalArgumentException, IllegalStateException or UnsupportedOperationException. This is referred to as failing-fast.

In your case I would throw an IllegalArgumentException as that's clearly what's happened - your method expects exactly two inputs; anything else is forbidden and you should fail-fast in such cases. Effective Java Item 38 also discusses this.

private static String method (String condition) {
  if (condition.equals("conditionOne")) {
    return value;
  } else if (condition.equals("conditionTwo")) {
    return symbol;
  }
  throw new IllegalArgumentException("Invalid condition '" + condition +"'");
}

Now you can be confident the only inputs this function will support are the ones it is designed to support. Even better, anyone calling your method incorrectly will get a clear, actionable error message.

The Guava User Guide has a good overview of different kinds of failures and when you should raise them.

You could also avoid this issue other ways - namely by defining a better method signature. It looks like you're defining a "stringly-typed" API; using an enum would help prevent callers from passing in erroneous parameters. See also Effective Java Items 50 and 30.

In rare cases (generally when dealing directly with user input) you'll want to fail-soft rather than fail-fast. This is common with confirmation dialogs; if you ask the user to enter "Yes" or "No" it's generally sufficient to simply check whether they entered "Yes" - whether they entered "No" or "Uhhhh", you'll just treat it as not-"Yes".

edited May 23 '17 at 11:51

Community

1
1

answered Jul 24 '16 at 22:25

dimo414

47,227
18
148
244

could you also place the thrown exception into an `else` block? – A is for Ambition Jul 24 '16 at 22:36
You could, but there's no need since the other blocks all `return`, so the `else` block is redundant. – dimo414 Jul 24 '16 at 22:38
so even though you may know that this exception may never be thrown, it is just generally safer to include it for the reasons explained above, right? – A is for Ambition Jul 24 '16 at 22:39
1

Right; if you return `null` or `""` you are *assuming* the `else` will never be reached; if you throw an exception you are *asserting* it is unreachable, and therefore can be confident your assumption is correct. Returning filler values is a classic source of really unpleasant bugs. – dimo414 Jul 24 '16 at 22:41
Regarding my currency program, I created a new `void` method that works perfectly. Once again, I needed to initialize the variables to avoid compiler error. Regarding good practices, with this `void` method, since I'm not returning anything, is it initializing the variables to an empty or `null` value fine? And since there's nothing to return, I don't need to throw a new exception for failing-fast, right? I've posted a new question (http://stackoverflow.com/q/38559519/5606164) for followup. I'd appreciate if you could provide your feedback once more. Thanks! – A is for Ambition Jul 25 '16 at 04:05
You should still throw an exception if your code's assumptions are violated (in this case, as an `else` block). This is a good practice regardless of anything else - what would your code do if someone passed in a currency other than USD or CNY? Even better, doing so here means you won't need to set the local variables to `null`. Here's a [cleaned up](http://pastebin.com/iwXbXFau) version of your function, but yes please post new questions rather than asking followups in comments. You'll get better answers. – dimo414 Jul 25 '16 at 04:13
You'd also avoid the whole issue by simply defining two separate methods - `convertUsdToCny()` and `convertCnyToUsd()` that each just take a `BigDecimal` parameter. – dimo414 Jul 25 '16 at 04:16
Let us [continue this discussion in chat](http://chat.stackoverflow.com/rooms/118182/discussion-between-a-is-for-ambition-and-dimo414). – A is for Ambition Jul 25 '16 at 06:53

Jossie Calderon · Answer 4 · 2016-07-24T22:04:01.650

0

If you've already guaranteed that the string will always equal either conditionOne or conditionTwo, then writing the 3rd else condition is not necessary.

It's equivalent to adding code that does not check if it's either conditionOne or conditionTwo. It's better off to remove it.

Edit Seeing your edit, I would recommend the above solution of returning "" since it's a string.

edited Jul 24 '16 at 22:04

answered Jul 24 '16 at 21:58

Jossie Calderon

1,393
12
21

please see the **Edit** in my post – A is for Ambition Jul 24 '16 at 22:03

Returning null in a method that accounts for multiple conditions

4 Answers4

Linked