CODEWITHSUNDEEP
java
user1406575
user1406575

Reputation: 1

Excessive use of If else statements

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

Upvotes: 0

Views: 264

Answers (4)

Kasper van den Berg
Kasper van den Berg

Reputation: 9526

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

Upvotes: 0

assylias
assylias

Reputation: 328608

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

Upvotes: 6

Tiago Peczenyj
Tiago Peczenyj

Reputation: 4623

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

Upvotes: 1

Oliver Charlesworth
Oliver Charlesworth

Reputation: 272497

Use a switch statement instead.

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

// ...

default:
    // default blah
    break;
}

Upvotes: 3

Related Questions