Modularize more efficiently in an if/else setup

Question

I wrote a program for one of my Object Oriented Programming in Java classes(Program 2) and I got docked off because my method was too long, which I'm not complaining about.

I got the note: "Modularize more, method too long."

I was wondering what any of you more experienced programmers would do to simplify/shorten my code for future sake so that I learn how to shorten this sort of code.

public void run() throws java.io.IOException
{
   stdin = new Scanner(System.in);
   String input;
   String fraction;
   int numPerLine;
   boolean inList = false;
   input = stdin.next(); // Get the read

  while(!input.equalsIgnoreCase("Q"))
  {
     if(input.equalsIgnoreCase("A"))
     {
        fraction = stdin.next(); // Get the fraction read
        thisFract = new Fraction(fraction);
        list.add(thisFract);
        System.out.println(thisFract.toString() + " was added "
              + "to the list.");
     }
     else if (input.equalsIgnoreCase("D"))
     {
        fraction = stdin.next();
        thisFract = new Fraction(fraction);
        inList = list.delete(thisFract);

        if(inList == true)
           System.out.println(thisFract.toString() + " was removed "
                 + "from the list.");
        else
           System.out.println(thisFract.toString() + " is not in the"
                 + " list.");
     }
     else if(input.equalsIgnoreCase("S"))
     {
        System.out.println("The sum of the list is: " + list.sum());
     }

Answer 1

There are some code smells in your code.

Duplicated Code:

fraction = stdin.next(); // Get the fraction read
thisFract = new Fraction(fraction);
list.add(thisFract);

fraction = stdin.next();
thisFract = new Fraction(fraction);
inList = list.delete(thisFract);

These lines do (almost) the same thing. It is commonly practice in software development to hold solutions at one place. In this case a method which reads the next fraction and another method which add a fraction to a list. This is meant by modularization. Divide your solution in little subsolutions which can be changed at one place and used at different places.

This code optimization is called Refactoring. There a lots of patterns for code refactoring.

Replace Conditional with Polymorphism:

You could think about using a switch-statement instead of the if-else blocks but it is highly recommended to use Polymorphism instead of conditional checks in object oriented programming languages.

Clean Code:

I would recommend also to read the book Clean Code written by Robert "Uncle Bob" C. Martin which explains lots of patterns to write well organized code.

For Example you can modularize your code like this:

1. move the if-else in one method
2. find code clones in the condition blocks and separate into methods

3. refactor methods top-down

   public void run() throws java.io.IOException {
       stdin = new Scanner(System.in);
       String input;
       input = nextFraction();
   
       while (!input.equalsIgnoreCase("Q")) {
           handleInput(input);
       }
   }
   
   private void handleInput(String input) {
       String fraction;
       int numPerLine;
       boolean inList = false;
   
       if (input.equalsIgnoreCase("A")) {
           handleA();      
       } else if (input.equalsIgnoreCase("D")) {
           handleD();      
       } else if (input.equalsIgnoreCase("S")) {
           handleS();          
       }
   }
   
   private void handleA() {
       assignNextFraction();
       addFractionToList();
       outputFractionAdded();      
   }
   
   private void handleD() {
       assignNextFraction();
       inList = deleteFractionFromList();
       if (inList) {
           outputFractionRemoved();
       } else {
           outputFractionNotInList();
       }   
   }
   
   private void handleS() {
       outputListSum();        
   }
   
   private boolean deleteFractionFromList() {
       boolean inList;
       inList = list.delete(thisFract);
       return inList;
   }
   
   private void addFractionToList() {
       list.add(thisFract);
   }
   
   private void assignNextFraction() {
       String fraction;
       fraction = nextFraction();
       thisFract = createFraction(fraction);
   }
   
   private void outputFractionAdded() {
       output(thisFract.toString() + " was added " + "to the list.");
   }
   
   private void outputFractionRemoved() {
       output(thisFract.toString() + " was removed " + "from the list.");
   }
   
   private Fraction createFraction(String fraction) {
       return new Fraction(fraction);
   }
   
   private String nextFraction() {
       String fraction;
       fraction = stdin.next(); // Get the fraction read
       return fraction;
   }
   
   private void outputFractionNotInList() {
       output(thisFract.toString() + " is not in the" + " list.");
   }
   
   private void outputListSum() {
       output("The sum of the list is: " + list.sum());
   }
   
   protected void output(String s) {
       System.out.println(s);
   }
   

Answer 2

To expand on Jan Koester's excellent answer, I would refactor the methods to a separate class. If done properly, this will allow you to write unit tests on the functionality. It will also make the switch statement you suggested better.

public class FractionSummingService {

    public void addFraction(string fraction) {
        // error check and add fraction to list here
    }

    public void removeFraction(string fraction) {
        // error check and remove fraction from list here
    }

    public string sumFractions()
    {
        // error check and sum list here
    }
}

Then you're main function looks something like this

public void run() throws java.io.IOException {
    stdin = new Scanner(System.in);

    FractionSummingService service = new FractionSummingService();
    while (true) {

        String input = nextFraction();

        switch(input) {  
            case 'A' :   
                service.addFraction(input);
                break;  
            case 'D' : 
                service.removeFraction(input);
                break;  
            case 'S' : 
                writeLine(service.sumFractions();
                break;
            case 'Q':
                // end loop
                break;
            default:
                writeUnknownCommand();
                break;
        }  
    }
}

to go one step further, you could put the valid commands into constants so your switch looks like this

switch(input) {     

    case ADD :   
        service.addFraction(input);
        break; 
    case DELETE : 
        service.removeFraction(input);
        break;  
    case SAVE : 
        writeLine(service.sumFractions();
        break;
    case QUIT:
        // end loop
        break;
    default:
        writeUnknownCommand();
        break;
    }

Answer 3

" wondering what any of you ... would do to simplify/shorten my code..."

How about you use Switch statements rather than those cumbersome cascade of if/else statements? That should make it cleaner and more readable.

Something like

switch(input) {  
    case 'X' :   
         break;  
    case 'Y' : 
         break;  
    case 'Z' : 
         break;
    default : 
         break;
}  

Answer 4

Just to add another view to already great answers. It is more about a different approach and personal preference than obviously better code.

A different approach that you can use could be a map (I tell you why I personally like it better) where your input would be a key and the value would a class implementing interface (let's) say FractionService.

public interface FractionService{
public void process(String input);

}

Then you could have three subclasses FractionAdditionService, FractionRemovalService, FractionIgnoreService. Those three would implement the process method and inside they would have your desired behavior that is described in your question and refined in the answers.

Then in the main class:

/*Better would be to use a map that when receiving a null as a key would return some dummy implementation that does nothing. This would prevent a NullPointerEx. But you get an idea about the map.*/
private static final Map<String, FractionService> mapOfServices = new HashMap<String,FractionService>();

static {
   mapOfServices.put("A", new FractionAdditionService());
   mapOfServices.put("D", new FractionRemovalService());
   mapOfServices.put("S", new FractionIgnoreService());

}

public void run() throws java.io.IOException {
    stdin = new Scanner(System.in);
    String input;
    input = nextFraction();

    while (!input.equalsIgnoreCase("Q")) {
        mapOfServices.get(input).process(input);
    }
}

An advantage with the map is that when you want to add another input letter and function for it you add one row here in your "manager" class and then the functionality into a separate class. Dowside, looking up in a map is slower than switch but you would care about that after you would find out that this is a bottleneck for you.

Then another suggestion that I would have is to pass the arguments into the methods where possible rather than using the instance variables. Particularly in @Jan Koester answer (his answer is great, dont get me wrong) I get slightly lost what is being assigned into the instance variable when and in which order. So to give you an example for one method:

private void handleA() {
    Fraction newFraction = assignNextFraction();
    addFractionToList(newFraction);
    outputFractionAdded(newFraction);      
}

Instead of assigning a value into "thisFract" in method "assignNextFraction" and then use it in the next methods you return it and then pass it as argument. For me it is better readable because I can better track what is flowing where and how it gets assigned. It is a notion from functional programming where ideally the functions (method) should not have any side effects (=playing around with non-local variables). Another advantage that I think it has is that you can test the functions better. You pass an argument and see the result, instead of having to set up instance variables in correct form and then test the result. Just a different approach, I personally like it better.

Answer 5

Per request, here is another approach and explanation. An interface was proposed; +1 the use of a Map was proposed. +1

Better yet is to put them together.

• For starters, we can leverage the fact that Enums can have an abstract method -- which is essentially an interface.
• Second, we can leverage the valueOf(someEnum.name()) to replace the Map proposed in other solutions.
• We can iterate through enums and print their toString() to handily show what we support.

try this (Use enums with abstract methods, skip the map and use 'valueOf'. Skip the interface):

public enum FractionService {

    A {
        @Override
        public void process(String input) {

        }
        @Override
        public String toString() {
            return "ADD";
        }
    },
    D {
        @Override
        public void process(String input) {

        }
        @Override
        public String toString() {
            return "DELETE";
        }
    },
    S {
        @Override
        public void process(String input) {

        }
        @Override
        public String toString() {
            return "SAVE";
        }
    }
    ,
    Q {
        @Override
        public void process(String input) {

        }

        @Override
        public String toString() {
            return "QUIT";
        }
    };


    public abstract void process(String input);

    public static void main(String[] args) {
        for (FractionService fractionService : FractionService.values()) {
            System.out.format("Commands are: %s: %s", fractionService.name(), fractionService.toString());

        }
        FractionService service = FractionService.valueOf("Q"); // results in QUIT
        service.process("foo");
    }
}