2

I've heard that it is bad practice to return null.

What are the alternatives to returning null in this case?

    public RollingStock getHeadPoint() {
        if (!train.isEmpty()) {
            return train.get(0);
        } else {
            return null;
        }
    }
SDJ
  • 4,083
  • 1
  • 17
  • 35
  • 2
    You could `throw new RuntimeException("No train available")` or similar... – deHaar Feb 20 '20 at 14:25
  • 11
    You could change the return type to `Optional` – khelwood Feb 20 '20 at 14:25
  • 2
    You could also ```throw new IllegalStateException("Train is empty")``` or follow the Java Spec guidelines where they throw ```NoSuchElementException``` e.g. https://docs.oracle.com/javase/7/docs/api/java/util/Deque.html#removeFirst() – kellyfj Feb 20 '20 at 14:25
  • 6
    @deHaar: Unchecked exception? That might not be the nicest thing to do – Hovercraft Full Of Eels Feb 20 '20 at 14:25
  • Does this answer your question? [java 8 optional to replace return null](https://stackoverflow.com/questions/43951727/java-8-optional-to-replace-return-null) – Julien Feb 20 '20 at 14:26
  • @HovercraftFullOfEels right, definitley not the nicest... – deHaar Feb 20 '20 at 14:26
  • @rhowell That's a very old topic and I think it's outdated, at least for `Java 8+`. Using `Optional` avoids NPEs and many different sorts of confusion. One may understand by a method's return type whether it is required or not. – ETO Feb 21 '20 at 18:48

3 Answers3

5

IMHO, the best option is to return an Optional<RollingStock>, like the foillowing:

public Optional<RollingStock> getHeadPoint() {
    if (!train.isEmpty()) {
        // or even Optional.ofNullable, if you are not sure 
        // whether train.get(0) is null or not 
        return Optional.of(train.get(0));  
    } else {
        return Optional.empty();
    }
}

Assuming train is a collection, as an alternative to manual wrapping the value into an Optional you could use Stream API:

public Optional<RollingStock> getHeadPoint() {
    return train.stream()
                .findFirst();
}

In some cases using inline train.stream().findFirst() may be more preferable than wrapping it into a separate method.

Once you already modified your method getHeadPoint to return Optional<RollingStock> you can use it as follows:

// ...
RollingStock headPoint = getHeadPoint().orElse(yourDefaultRollingStock);
// or
RollingStock headPoint = getHeadPoint().orElseGet(aMethodGettingYourDefaultRollingStock());
// or
RollingStock headPoint = getHeadPoint().orElseThrow(() -> new Exception("The train is empty!"));
// or
getHeadPoint().ifPresent(headPoint -> doSomethingWithHeadPoint(headPoint));
ETO
  • 6,970
  • 1
  • 20
  • 37
0

You should differentiate "get" and "search/find" methods :

  • getHeadPoint : some train should exist. If not, it's against your business, throw an exception
  • findHeadPoint : non-existence of the train is coherent to your business, return null
greyxit
  • 693
  • 3
  • 13
0

You could define a TrainIsEmptyException and throw that when the train is empty. Make it a checked Exception. Or you can just return null.

Christine
  • 5,617
  • 4
  • 38
  • 61