java Refactoring if else

Question

I have this if else code, I was wondering if there is more useful/intelligent way for writing it :

public void saveContent() throws Exception {
   if(book.isColored()) {
      book.setChoosen(“1234”);
   } else if (book.isAvailable()) {
      book.setChosen(“23498”);
   } else if (book.isAdults()) {
      book.setChosen(“0562”);
   } else {
      ReaderResponse response = reader.getReaderResponse();
      if (response != null) {
         book.setChosen(response.getName());
         }
      } else {
            book.setChosen(“4587”);
      }
   }
}

The method returns void.

@parladneupane how do you use switch on different attributes? — jhamon
– jhamon, Commented Jan 7, 2019 at 13:14
Just out of interest: Can't a book be colored, available and adult-only? We have available, colored adult-only books in my country. — deHaar
– deHaar, Commented Jan 7, 2019 at 13:15
If those booleans are exclusive, then it could be a single value with an enum, and therefore a switch. From their names, they don't sound as if they are, which makes me wonder if your if else code really makes a lot of sense. I mean, you care about whether or not the book is available only if it is not colored? — Zag
– Zag, Commented Jan 7, 2019 at 13:16
As written, this wouldn't even compile. Your braces are messed up, and your last else is not attached to any if. — khelwood
– khelwood, Commented Jan 7, 2019 at 13:23

Tom Hawtin - tackline · Accepted Answer · 2019-01-07 13:20:40Z

5

The introduction of a local variable in the middle of this causes problems. One way of dealing this is introducing another method - don't be afraid of small methods.

public void saveContent() throws Exception {
    book.setChoosen(
        book.isColored()   ? “1234"  :
        book.isAvailable() ? “23498” :
        book.isAdults()    ? “0562”  :
        readerResponse()
    );
}
private String readerResponse() throws Exception {
    ReaderResponse response = reader.getReaderResponse();
    return response == null ? “4587” : response.getName();
}

? : is the conditional operator, often referred to as the ternary operator.

In the event that getReaderResponse has no side-effect, you could repeat the call. get methods typically do not have side-effects, but I get the feeling here this one may well do. I am not sure where the Exception is thrown - I assume that is intended to be replace with a subtype.

edited Jan 7, 2019 at 13:20

answered Jan 7, 2019 at 13:17

Tom Hawtin - tackline

148k30 gold badges222 silver badges314 bronze badges

Sign up to request clarification or add additional context in comments.

2 Comments

tobias_k Over a year ago

Very nice. How about Optional.ofNullable(reader.getReaderResponse).map(r -> r.getName()).orElse("4587")?

tobias_k Over a year ago

Not at all, just wanted to suggest. Your "long" way might indeed be more readable.

dic19 · Accepted Answer · 2019-01-07 13:24:43Z

I wouldn't say this is more efficient, more useful or smarter way of doing the same, but certainly I think the code will be cleaner and testable by delegating the actual choosen code resolution to a different method:

public void saveContent() throws Exception {
   String choosenCode = getChoosenCode(book);
   book.setChoosen(choosenCode);
}

static String getChoosenCode(Book book) throws Exception {
   if (book.isColored()) {
      return “1234”;
   }
   if (book.isAvailable()) {
      return “23498”;
   }
   if (book.isAdults()) {
      return “0562”;
   }
   ReaderResponse response = reader.getReaderResponse();
   return (response != null) 
      ? response.getName()
      : “4587”;
}

As you can see there is no need of endless if-else blocks because of the early return approach. This also provides the ability to unit-test the choosen code part, which is a little more complicated if you have a void returning method.

pobu · Accepted Answer · 2019-01-07 13:19:10Z

1

You can implement Strategy Design Pattern as an alternative to if-else construction

answered Jan 7, 2019 at 13:19

pobu

4021 gold badge5 silver badges14 bronze badges

2 Comments

theo Over a year ago

I tend to like Strategy Pattern a lot. Don't you think it will be overkill for such a simple case ?

pobu Over a year ago

Maybe you're right, but strategy bases on lightweight classes. It will not have much impact.

Rauf Aghayev · Accepted Answer · 2019-01-07 13:17:31Z

0

Switch statement wouldn't help, because you're checking different kind of attributes. I'd use if statements but instead of "magical numbers" I'd use some static final constants. And I'd remove inside of last else statement into a separate method.

answered Jan 7, 2019 at 13:17

Rauf Aghayev

3001 silver badge13 bronze badges

Comments

O. Schnieders · Accepted Answer · 2019-01-07 13:23:11Z

0

Well, this is a issue i´m thinking about for months. In my opinion it is not easy to write that code in a way it is smart and reabable for non senior programmers. A good way would be using a switch-statement, but this is not useable for boolean statements.

I would use the code as mentioned in the questions, but i´d move it to a single class/controller/service for more complex code (this would be better for testing complex behaviours).

answered Jan 7, 2019 at 13:23

O. Schnieders

6811 gold badge9 silver badges17 bronze badges

Comments

Joop Eggen · Accepted Answer · 2019-01-07 13:30:01Z

Just wanted to add a Stream version, but I do not consider it adequate for just a handfull of cases.

List<Pair<Predicate<Book>, String>> mapping;
mapping.add(Book::isColored, "1234");
mapping.add(Book::isAvailable, "23498");
mapping.add(Book::isAdults, "0562");
ReaderResponse response = reader.getReaderResponse();
mapping.add((anyBook) -> response == null, "4587");

String chosen = mapping.stream()
        .filter(p -> p.getKey().test(book))
        .map(Pair::getValue)
        .findFirst()
        .orElseGet(response::getName);

Or

mapping.add((anyBook) -> true, response.getName());
mapping.stream()
        .filter(p -> p.getKey().test(book))
        .map(Pair::getValue)
        .findFirst()
        .ifPresent(book::setChose);

OldCurmudgeon · Accepted Answer · 2019-01-07 14:10:21Z

This doesn't implement your algorithm exactly (because there are some ambiguities) but demonstrates using an enum as a strategy pattern. In this form it is much easier to add/adjust the code because all special functionality of each book type is in it's own location (it's own enum).

Note that this also helps when a book can fall into several types (such as Coloured and Adult).

enum BookType {
    Coloured("1234"),
    Available("23498"),
    Adult("0562");

    private final String chosen;

    BookType(String chosen) {
        this.chosen = chosen;
    }

    public String getChosen() {
        return chosen;
    }

}

class Book {
    final Set<BookType> types = EnumSet.noneOf(BookType.class);
    Book book;

    public boolean hasType(BookType type) {
        return types.contains(type);
    }

    public void setChosen(String s) {

    }
}

public void saveContent() throws Exception {
    for(BookType type: BookType.values()) {
        if(book.hasType(type)) {
            book.setChosen(type.getChosen());
        }
    }
}

Collectives™ on Stack Overflow

java Refactoring if else

7 Answers 7

2 Comments

Comments

2 Comments

Comments

Comments

Comments

Comments

Your Answer

Hot Network Questions

Collectives™ on Stack Overflow

7 Answers 7

2 Comments

Comments

2 Comments

Comments

Comments

Comments

Comments

Your Answer

Sign up or log in

Post as a guest

Related