4

I don't find the appropriate solution for my question. As far as I know returning null is a not a good way to write clean code, as the book Clean Code says. However there are many different opinions about this practice, and I am not sure about which one fits on my function.

private EArea getSimilarExistingArea(EImportArea importedArea) {
    for (EArea existingArea : exsitingAreas) {
        EList<EPoint> importedAreaPoints = importedArea.getPoly().getPoints();
        EList<EPoint> existingAreaPoints = existingArea.getPoly().getPoints();
        for (EPoint importedAreaPoint : importedAreaPoints) {
            for (EPoint existingAreaPoint : existingAreaPoints) {
                if (importedAreaPoint.equals(existingAreaPoint))
                    return existingArea;
            }
        }
    }
    return null;
}

What should I return if there is not an existing similar area?

PD: For optimize my code I am breaking the loops with the return if an existing area is founded.

Alex Blasco
  • 793
  • 11
  • 22
  • depends but if you are returning null your code should also know how to handle null. Like in your case if `getSimilarExistingArea` function return null then you can print some fancy message to the user – Lokesh Pandey Oct 25 '17 at 09:51
  • 1
    `Exception` are a good alternatives, but in your example, a good javadoc pointing the `null` if not found is acceptable too. – AxelH Oct 25 '17 at 09:53
  • 2
    Have a read of this question: [Is returning null bad design?](https://stackoverflow.com/questions/1274792/is-returning-null-bad-design/1274822#1274822). In my opinion (as this question is largely opinion based), returning `null` is fine, as long as it is well documented. If returning `null` implies something is very wrong, then an exception should be thrown. Ask yourself what the pre and post conditions of the method are. – d.j.brown Oct 25 '17 at 09:53
  • 1
    For private method, it is up to you to return anything, in case you have handled it inside your own class. For public method, you should stated the possible returning values including null with reason, then it should be fine. – Alex Oct 25 '17 at 09:53
  • Looking at this answer may allow you to consider if returning null is a bad design for the API users : https://stackoverflow.com/questions/271526/avoiding-null-statements/271874#271874. Possibly a duplicate of https://stackoverflow.com/questions/1274792/is-returning-null-bad-design – bric3 Oct 25 '17 at 10:52
  • Possible duplicate of [Is returning null bad design?](https://stackoverflow.com/questions/1274792/is-returning-null-bad-design) – bric3 Oct 25 '17 at 10:53

2 Answers2

5

You should take a look at the Optional class!

Make your method return type Optional<EArea> and simply return Optional.ofNullable(existingArea) you will have to slightly modify your Code but the benefits of Optional are really worth it!

Dinh
  • 759
  • 5
  • 16
  • 1
    Or when not finding `Optional.empty()`, when finding `Optional.of(...)`. – Joop Eggen Oct 25 '17 at 09:57
  • 1
    And Optional is available since Java 1.8 – Shubhendu Pramanik Oct 25 '17 at 10:05
  • 1
    @JoopEggen that is the purpose of Optional#ofNullable static Optional ofNullable(T value) Returns an Optional describing the specified value, if non-null, otherwise returns an empty Optional. – Dinh Oct 25 '17 at 10:37
  • 1
    @Dinh that was just an additional comment as the OP's code had two returns: null and non-null. No criticism, even gave +1. – Joop Eggen Oct 25 '17 at 10:54
  • Thank you both. I used Optional class [Optional class](https://docs.oracle.com/javase/8/docs/api/java/util/Optional.html) to solve my problem. – Alex Blasco Oct 25 '17 at 12:02
1

Finally I used Optional Class to solve my problem.

Here is the code:

private Optional<EArea> getSimilarExistingArea(EImportArea importedArea) {
    for (EArea existingArea : baseLineService.getBaseLine().getAreas()) {
        EList<EPoint> importedAreaPoints = importedArea.getPoly().getPoints();
        EList<EPoint> existingAreaPoints = existingArea.getPoly().getPoints();
        for (EPoint importedAreaPoint : importedAreaPoints) {
            for (EPoint existingAreaPoint : existingAreaPoints) {
                if (importedAreaPoint.equals(existingAreaPoint))
                    return Optional.of(existingArea);
            }
        }
    }
    return Optional.empty();
}

And here is how I check the returned value:

if (getSimilarExistingArea(importedArea).isPresent())
Alex Blasco
  • 793
  • 11
  • 22