CODEWITHSUNDEEP
javaif-statementcoding-efficiency
userit1985
userit1985

Reputation: 993

java Refactoring if else

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.

Upvotes: 0

Views: 163

Answers (7)

OldCurmudgeon
OldCurmudgeon

Reputation: 65793

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());
        }
    }
}

Upvotes: 0

Joop Eggen
Joop Eggen

Reputation: 109547

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);

Upvotes: 0

dic19
dic19

Reputation: 17971

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.

Upvotes: 2

O. Schnieders
O. Schnieders

Reputation: 681

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).

Upvotes: 0

Tom Hawtin - tackline
Tom Hawtin - tackline

Reputation: 147124

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.

Upvotes: 5

pobu
pobu

Reputation: 402

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

Upvotes: 1

Rauf Aghayev
Rauf Aghayev

Reputation: 300

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.

Upvotes: 0

Related Questions