How to avoid a lot of if else conditions

Question

I have read a lot of topics about code refactoring and avoiding of if else statements. Actually, I have a class where I am using a lot of if - else conditions.

More details: I am using the pull parser and on each line of my soap response, I will check if there is a tag I am interested on, if not, check another tag etc:

 if(eventType == XmlPullParser.START_TAG) {
            soapResponse= xpp.getName().toString();
            
            if (soapResponse.equals("EditorialOffice")){  
                eventType = xpp.next();
                if (xpp.getText()!=null){
                editorialOffice += xpp.getText();
                }
            }   
            else if (soapResponse.equals("EditorialBoard")){  
                eventType = xpp.next();
                if (xpp.getText()!=null){
                editorialBoard += xpp.getText();
                }
            }
            else if (soapResponse.equals("AdvisoryBoard")){  
                eventType = xpp.next();
                if (xpp.getText()!=null){
                advisoryBoard += xpp.getText();
                }
            }   
        }
        eventType = xpp.next();
     }

Now, I would like to use something else, instead of those if else conditions, but I don't know what.

Can you please give me an example?

Answer 1

In this particular case, since the code is essentially identical for all 3 case except for the String being appended to, I would have a map entry for each of the Strings being built:

Map<String,String> map = new HashMap<String,String>();
map.put("EditorialOffice","");
map.put("EditorialBoard","");
map.put("AdvisoryBoard","");
// could make constants for above Strings, or even an enum

and then change your code to the following

if(eventType == XmlPullParser.START_TAG) {
    soapResponse= xpp.getName().toString();
    String current = map.get(soapResponse);
    if (current != null && xpp.getText()!=null) {
        map.put( soapResponse, current += xpp.getText());
    }
    eventType = xpp.next();
}

No "if... then... else". Not even the added complexity of multiple classes for strategy patterns, etc. Maps are your friend. Strategy is great in some situations, but this one is simple enough to be solved without.

Answer 2

You can define an enum like the following:

public enum SoapResponseType {
    EditorialOffice(1, "description here") {
        public void handle(XmlPullParser xpp) {
            //do something you want here
            return null;
        }
    },
    EditorialBoard(2, "description here") {
        public void handle(XmlPullParser xpp) {
            //do something you want here
            return null;
        }
    },
    AdvisoryBoard(3, "description here") {
        public void handle(XmlPullParser xpp) {
            //do something you want here
            return null;
        }
    };

    public static SoapResponseType nameOf(String name) {
        for (SoapResponseType type : values()) {
            if (type.getName().equalsIgnoreCase(name)) {
                return type;
            }
        }
        return null;
    }

    public void handle(XmlPullParser xpp) {
        return null;
    }
}

Use above enum like this:

SoapResponseType type = SoapResponseType.nameOf("input string");
if (type != null) {
    type.handle(xpp);
}

It's clean code, isn't it!

Answer 3

Try to look at the strategy pattern.

• Make an interface class for handling the responses (IMyResponse)
  • Use this IMyResponse to create AdvisoryBoardResponse, EditorialBoardResponse classes
• Create an dictionary with the soapresponse value as key and your strategy as value
• Then you can use the methods of the IMyResponse class by getting it from the dictionary

Little Example:

// Interface
public interface IResponseHandler {
   public void handleResponse(XmlPullParser xxp);

}

// Concrete class for EditorialOffice response
private class EditorialOfficeHandler implements IResponseHandler {
   public void handleResponse(XmlPullParser xxp) {
       // Do something to handle Editorial Office response
   }
}

// Concrete class for EditorialBoard response
private class EditorialBoardHandler implements IResponseHandler {
   public void handleResponse(XmlPullParser xxp) {
       // Do something to handle Editorial Board response
   }
}

On a place you need to create the handlers:

Map<String, IResponseHandler> strategyHandlers = new HashMap<String,IResponseHandler>();
strategyHandlers.put("EditorialOffice", new EditorialOfficeHandler());
strategyHandlers.put("EditorialBoard", new EditorialBoardHandler());

Where you received the response:

IResponseHandler responseHandler = strategyHandlers.get(soapResponse);
responseHandler.handleResponse(xxp);

Answer 4

You haven't mentioned if you can or do use Java 7. As of that java version you can use Strings in switch statements.

Other than that, encapsulating the logic for each case is a good idea, for example:

Map<String, Department> strategyMap = new HashMap<String, Department>();
strategyMap.put("EditorialOffice", new EditorialOfficeDepartment());
strategyMap.put("EditorialBoard", new EditorialBoardDepartment());
strategyMap.put("AdvisoryBoard", new AdvisoryBoardDepartment());

Then you can simply select the correct strategy from the Map and use it:

String soapResponse = xpp.getName();
Department department = strategyMap.get(soapResponse);
department.addText(xpp.getText());

Department is of course in interface...

Answer 5

You could create a ResponseHandler interface with three implementations, one for each branch of your if/else construct.

Then have either a map mapping the different soapResponses to a handler, or a list with all the handler if it can handle that soapResponse.

You also should be able to move some of the boilerplate code to a common possibly abstract implementation of the response handler class.

As so often there a many variations of this. By utilizing the code duplication one actually needs only one implementation:

class ResponseHandler{
    String stringToBuild = "" // or what ever you need
    private final String matchString

    ResponseHandler(String aMatchString){
        matchString = aMatchString
    }
    void handle(XppsType xpp){
        if (xpp.getName().toString().equals(matchString){
            eventType = xpp.next();
            if (xpp.getText()!=null){
                 editorialOffice += xpp.getText();
            }
        }
    }
}

Your code becomes

List<ResponseHandler> handlers = Arrays.asList(
    new ResponseHandler("EditorialOffice"),
    new ResponseHandler("EditorialBoard"),
    new ResponseHandler("AdvisoryBoard"));
if(eventType == XmlPullParser.START_TAG) {
    for(ResponseHandler h : handlers)
        h.handle(xpp);
}

Answer 6

vast question that is this one and there is no real answer. (and I don't use soap very often)

here a just some ideas based on your code:

first you can groupe duplicate code

if (soapResponse.equals("EditorialOffice")
||soapResponse.equals("EditorialBoard")
||soapResponse.equals("AdvisoryBoard")){ 

Another good thing you can do is play around with switch staments like:

switch(soapResponse){
case "EditorialOffice":
case "EditorialBoard":
case "AdvisoryBoard":
eventType = xpp.next();
                if (xpp.getText()!=null){
                advisoryBoard += xpp.getText();
                }
break;

Also you should consider breaking down you test into small functions:

public bool interestingTag(string s){
return (soapResponse.equals("EditorialOffice")
    ||soapResponse.equals("EditorialBoard")
    ||soapResponse.equals("AdvisoryBoard"));
}

    public processData(xpp){
    eventType = xpp.next();
                    if (xpp.getText()!=null){
                    editorialBoard += xpp.getText();
                    }
    ....}

So that you can just process all your answers in a while loop and you super long if else becomes a 5~10 line function

But as I said there are so many good ways of doing the same thing

Answer 7

In Java 7 you can SWITCH on Strings. You could use that if you could use that ;-)

Answer 8

Besides zzzzzzz(etc.)'s comment... keep in mind that you are using XmlPullParser which makes you write ugly code like the one you have. You could register some callbacks that would split your code and make it 'better', but if possible, just use SimpleXML library or similar.

Also, you can refactor your code to make it more readable and less verbose. For instance, why do you call xpp.next() inside each if statement? Why not just calling it outside just once:

if(eventType == XmlPullParser.START_TAG) {
    soapResponse= xpp.getName().toString();
    if (soapResponse.equals("EditorialOffice") && xpp.getText()!=null){  
        editorialOffice += xpp.getText();
    }   
    else if (soapResponse.equals("EditorialBoard") && xpp.getText()!=null){  
        editorialBoard += xpp.getText();
    }
    else if (soapResponse.equals("AdvisoryBoard") && xpp.getText()!=null){  
        advisoryBoard += xpp.getText();
    }   
}
eventType = xpp.next();