Excessive use of If else statements

Question

I have one query that is I have used a method but there is many time I have used If Else ..not it become very ambiguous please advise can I use some other conditional loop also..below is my code..

 if (cardType == AARP_CARD_TYPE) {
      userResponse = messageBox.showMessage("CandidateAARPCardAttachCardToExistingTransaction",
          null, IMessageBox.YESNO); // MSG:31.59
      transaction.setValue(ITransactionHashtableWag.LOYALTY_MESSAGE_DISPLAYED,
          WalgreensRewardsConstants.ATTACH_CANDIDATE_AARP_CARD);
    } else if ((cardType == PSC_CARD_TYPE) && ((!PosHelper.isRunningAsService()))) {
      userResponse = messageBox.showMessage("PendingPSCCardAttachCardToExistingTransaction", null,
          IMessageBox.YESNO); // MSG:31.60
      transaction.setValue(ITransactionHashtableWag.LOYALTY_MESSAGE_DISPLAYED,
          WalgreensRewardsConstants.ATTACH_PENDING_PSC_CARD);

    } else if ((cardType == DR_CARD_TYPE) && ((!PosHelper.isRunningAsService()))) {
      userResponse = messageBox.showMessage("PendingDRCardAttachCardToExistingTransaction", null,
          IMessageBox.YESNO); // MSG:31.63
      transaction.setValue(ITransactionHashtableWag.LOYALTY_MESSAGE_DISPLAYED,
          WalgreensRewardsConstants.ATTACH_PENDING_DR_CARD);

    } else if ((cardType == WAG_LOYALTY_CARD_TYPE)){
                transaction.setValue(ITransactionHashtableWag.LOYALTY_MESSAGE_DISPLAYED,
                  WalgreensRewardsConstants.ATTACH_NOT_ON_FILE);

            if((!PosHelper.isRunningAsService())) {
      userResponse = messageBox.showMessage("CardNotOnFileToAttach", null, IMessageBox.YESNO); // MSG:31.32
      // BUC
      // 1.22.1
    }


    } else { // If the device is neither of these, POS displays Message 1
      // Button, MSG 31.14. [BUC
      // 1.23.2]
      displayMessage("InvalidLoyaltyCard");
      transaction.setValue(ITransactionHashtableWag.LOYALTY_MESSAGE_DISPLAYED,
          NOT_VALID_LOYALTY_CARD);
      userResponse = -1;
    }

Please advise how can I improve my above logic with some other conditional statements as there is lots n lots of If Else is used..!!

You seem to prefer the `switch` alternative - it's your call. But as you are in refactoring mode, it would make sense to consider whether the current pattern of using byte constants is the most efficient - unless you have no control over that part of the code that defines those constants. — assylias, May 23 '12 at 17:18

assylias · Answer 1 · 2012-05-23T17:16:18.463

If cardType is an enum, you can add methods to your enum, (say getName, getWag etc.) and call it:

userResponse = messageBox.showMessage(cardType.getMessage(), ...
transaction.setValue(cardType.getWag(), cardType.getRewards());

If it is an int or another non-enum type, you can use a switch as already proposed, or consider switching (haha) to an enum. You could also make PosHelper.isRunningAsService() a boolean parameter to those methods and all your if/else code would be reduced to 3 or 4 lines it seems (although it will introduce some coupling but you seem to have a lot of it already).

Your enum could look like this (simple example that you can complicate as required):

public enum CardType {
    AARP_CARD_TYPE {
        public String getName() {
            return "CandidateAARPCardAttachCardToExistingTransaction";
        }
    },
    PSC_CARD_TYPE {
        public String getName() {
            return "PendingPSCCardAttachCardToExistingTransaction";
        }
    };

    public abstract String getName();
}

Or more compact, if you don't require complicated logic in the methods:

    public static enum CardType {
        AARP_CARD_TYPE("CandidateAARPCardAttachCardToExistingTransaction"),
        PSC_CARD_TYPE ("PendingPSCCardAttachCardToExistingTransaction");

        private final String transactionName;

        CardType(String transactionName) {
            this.transactionName = transactionName;
        }

        public String getName() {
            return transactionName;
        }
    }

score 3 · Answer 2 · answered May 23 '12 at 16:56

3

Use a switch statement instead.

switch (cardType) {
case AARP_CARD_TYPE:
    // blah
    break;
case PSC_CARD_TYPE:
    // blah
    break;

// ...

default:
    // default blah
    break;
}

answered May 23 '12 at 16:56

Oliver Charlesworth

267,707
33
569
680

1

Which won't necessarily help, because some of those have multiple conditions to check. – Clockwork-Muse May 23 '12 at 16:58
@X-Zero: True. But these can be nested inside the individual cases. So it will help in the sense that it greatly reduces the verbosity of the code. – Oliver Charlesworth May 23 '12 at 16:59
@Oli..here the card type is of byte will it work, thanks in advance – user1406575 May 23 '12 at 17:05
@oli..Just check frm case 2 I have an or condiotion also..lse if ((cardType == PSC_CARD_TYPE) && ((!PosHelper.isRunningAsService()))) { – user1406575 May 23 '12 at 17:15
@OliCharlesworth Please advise..!! – user1406575 May 23 '12 at 17:21
you used `!PosHelper.isRunningAsService()` few times. write one `if` with this statement and in other cases use `switch-case`. Also you can write few cases with one action – alaster May 23 '12 at 17:23

score 1 · Answer 3 · answered May 23 '12 at 17:01

You have some options: Pattern Strategy, Polymorphism or Events to avoid too much ifs/else

In your example probably the business logic is close to the user interface. You can use the MVC concept to separate the logic from the presentation and reduce the if/elses (if possible).

score 0 · Answer 4 · edited May 23 '17 at 10:27

0

If you don't like adding methods to CardType as assylias suggested, you can create an 'Action' enum and add the method(s) to that one and use a Map

edited May 23 '17 at 10:27

Community

1
1

answered May 23 '12 at 17:20

Kasper van den Berg

8,951
4
48
70

Excessive use of If else statements

4 Answers4

Linked

Related