2

I would like to ask if it is a good way to have many else if statements based on the boolean conditions like below?

public void borrowItem() throws IOException {

    boolean ableToBorrow = isUserAbleToBorrow(cardID);
    boolean isDemand = checkDemand(title, authorNumber);
    boolean userExists = checkIfUserExists(cardID);


    if(ableToBorrow && isDemand && userExists) {

           //lots of code....

    }else if(!ableToBorrow && isDemand && userExists) {
        System.out.println("User limit  exceeded");
    }else if(!ableToBorrow && !isDemand && userExists) {
        System.out.println("User limit  exceeded and no book demand");
    }else if(ableToBorrow && !isDemand && userExists) {
        System.out.println("No book demand");
    }else if(ableToBorrow && !isDemand && !userExists) {
        System.out.println("No book demand and user does not exists");
    }else if(ableToBorrow && isDemand && !userExists) {
        System.out.println("Unrecognized user!");
    }

}

Is it a good way or there is a better idea in java to do that?

8
  • 2
    What's "good"? As long as your code is readable, nothing wrong with multiple if-else.
    – Maroun
    Apr 29, 2018 at 14:29
  • 2
    You could fragment it in a hierarchical way, like if (ableToBorrow) and inside that if (isDemand) and inside that if (userExists). There is probably a better way which I don't see for now.
    – Lutzi
    Apr 29, 2018 at 14:31
  • 1
    No, it's not good. It is hard to read the code and also hard to understand what each condition does and if every possible combination of true/false is covered. Apr 29, 2018 at 14:33
  • 2
    @Maroun Ah, no. Simply no. Lengthy if-else chains are (almost) never a good thing. You avoid them where possible.
    – GhostCat
    Apr 29, 2018 at 14:33
  • 2
    @GhostCat Unless you're a researcher ;) Seriously, sometimes over OOPing thing leads to bad code. If it's a bunch of 4-5 if-else, I can live with it easily.
    – Maroun
    Apr 29, 2018 at 14:33

3 Answers 3

2

I agree with what GhostCat wrote. It's way too procedural. One way (probably the best way in this case) to implement polymorphism would be the decorator pattern.

Define your interface:

public interface User {
    void borrowItem(String item);
    String cardId();
}

Create the base implementation:

public final class SimpleUser implements User {
    private final String cardId;
    public SimpleUser(final String cardId) {
        this.cardId = cardId;
    }
    @Override
    public void borrowItem(final String item) {
        // Borrowing logic.
    }
    @Override
    public String cardId() {
        return cardId;
    }
}

And then add decorators for each validation you need. E.g. to check if user exists:

public final class ExistingUser implements User {
    private final User origin;
    public ExistingUser(final User origin) {
        this.origin = origin;
    }
    @Override
    public void borrowItem(final String item) {
        if (!exists(cardId())) {
            throw new IllegalStateException("Unrecognized user!");
        }
        origin.borrowItem(item);
    }
    @Override
    public String cardId() {
        return origin.cardId();
    }
    private boolean exists(String cardId) {
        // Check if exists...
    }
}

And combine them. This way, when you need one additional validation, you add one additional decorator. With ifs, the number of cases would grow geometrically.

new ExistingUser(
    new DemandAwareUser(
        new SafelyBorrowingUser(
            new SimpleUser(cardId)
        )
    )
).borrowItem(item);
1
  • I will try, thanks :)
    – Dawid
    Apr 29, 2018 at 16:54
2

It is very bad style: hard to read and understand, easy to mess up when you are asked to enhance/change behavior. Please note that such code is also extremely hard to test - as you would want to make sure to cover all possible paths that flow can take within such a metho.

The typical answer to such things is to use polymorphism, like having a base class defining some interface, and specific child classes each implementing the interface differently.

In that sense: your code is a clear violation of Tell Don't Ask: you code queries some status from somewhere, to then make decisions on that. Instead, you create classes/objects and tell them to do the right thing (again: that is where polymorphism kicks in).

-1

Nothing wrong with the way it is, but there are other options if you want your code to be more concise. If I change you error messages very slightly, I can write it this way:

if(ableToBorrow && isDemand && userExists) {
   //lots of code....
} else {
  String errorMsg = "";
  if (!ableToBorrow) errorMsg += "User limit  exceeded - ";
  if (!isDemand) errorMsg += "No book demand - ";
  if (!userExists) errorMsg += "Unrecognized user!"
  System.out.println(errorMsg);
} 

There is also the option of bundling the boolean's into a single integer value. This obfuscates what your code is doing though, and I personally wouldn't use unless you wanted to create an enum to keep track of what the integer values meant;

int status = 4*(ableToBorrow?1:0)+2*(isDemand?1:0)+(userExists?1:0);
switch(status){
  case 7:  //lots of code....
    break;
  case 3:  System.out.println("User limit  exceeded");
    break;
  //etc...
}
1
  • 1
    int status ... is (will be) nice trick in C/drivers code, but hard to acceptable here
    – Jacek Cz
    Apr 29, 2018 at 14:50

Not the answer you're looking for? Browse other questions tagged or ask your own question.