CODEWITHSUNDEEP
javacode-cleanup
Triinu86
Triinu86

Reputation: 45

Cleaning up java code, multiple if statements

I'm supposed to clean up a java code, got rid of lots of stuff, but should is there anything else to clean, maybe somehow get rid of multiple if statements, without totally rewriting this code? Can't seem to figure it out as they are so different to stack them in one 'if'. Any ideas?

 public class Calc {
           // employee types
           public static final int SELLER;
           public static final int COOK;
           public static final int CHIEF;



       public static void main(final String[] args) {
              Calc c = new Calc();
              System.err.println(c.pay(CHIEF) + " should be 66");
       }

       private int pay(final int type, final int h) {
              int Sum = 0;
              if (type == SELLER) {
                     if (h > 8) { 
                           Sum = 20 * (h - 8);  
                           Sum += 80;
                     } else {
                           Sum += 10 * h;
                     }
              }
              if (type == COOK) {
                     if (h > 8) { 
                           Sum = 30 * (h - 8); 
                           Sum += 15 * 8;
                     } else {
                           Sum += 15 * h;
                     }
              }
              if (type == CHIEF) {
                     if (h > 8) { 
                           Sum = 66 * (h - 8); 
                           Sum += 22 * 8;
                     } else {
                           Sum += 22 * h;
                     }
              }
              if (h > 20) {
                     if (type == SELLER) {
                           Sum += 10;
                     }
                     if (type == COOK) {
                           Sum += 20;
                     }
                     if (type == CHIEF) {
                           Sum += 30;
                     }
              }
              return Sum;
       }
}

Upvotes: 0

Views: 2152

Answers (5)

Abin Roy
Abin Roy

Reputation: 9

Write a switch block for the type inside pay function instead of using multiple if else statement. Instead of giving type == SELLER , create a proper name and do the comparison.

Also an interface will be good for the Employees, with a factory pattern to get the object.

For code cleanup you can also add in plugins like: https://www.sonarlint.org/

Upvotes: -1

jpllosa
jpllosa

Reputation: 2202

Here's another way of cleaning up or implementing it with minimal if statements in the pay method. Move the employee types to an enum and then per enum type has its own pay calculation. Like so:

package sandbox;

public enum Employee {
    SELLER() {
        @Override 
        int pay(int h) {
            int sum = (h > 20) ? 10 : 0;
            if (h > 8) { 
                sum = 20 * (h - 8);  
                sum += 80;
            } else {
                sum += 10 * h;
            }
            
            return sum;
        }
    },
    COOK() {
        @Override 
        int pay(int h) {
            int sum = (h > 20) ? 20 : 0;
            if (h > 8) { 
                sum = 30 * (h - 8);  
                sum += 15 * 8;
            } else {
                sum += 15 * h;
            }
            
            return sum;
        }
    },
    CHIEF() {
        @Override 
        int pay(int h) {
            int sum = (h > 20) ? 30 : 0;
            if (h > 8) { 
                sum = 66 * (h - 8);  
                sum += 22 * 8;
            } else {
                sum += 22 * h;
            }
            
            return sum;
        }       
    };
    
    abstract int pay(int h);
}

And the runner...

package sandbox;

public class Main {

    public static void main(String[] args) {
        System.out.println(Employee.SELLER.pay(5));
        System.out.println(Employee.COOK.pay(5));
        System.out.println(Employee.CHIEF.pay(5));
    }
}

Upvotes: 0

Andrii Lohvynchuk
Andrii Lohvynchuk

Reputation: 61

The code you wrote is purely procedural and in most cases is considered a bad practice while writing in Object Oriented language like Java. You should learn about the power of polymorphism and should not perform a type check manually:

if (type == COOK) { //Avoid doing this in OO languages!

You should think of your domain entities (employees) as objects and each specific type of employee can define its own rules for calculating pay.

Let's create an abstract class Employee with single abstract method int calculatePay(int h):

public abstract class Employee {
    abstract int calculatePay(int h);
}

Word abstract means that this method has no actual implementation but all the logic for calculating pay will be put in the subclasses Seller, Cook and Chief:

public class Cook extends Employee {

    public Cook() {}

    int calculatePay(int h) {
        int sum = (h > 20) ? 20 : 0;
        if (h > 8) {
            sum = 30 * (h - 8);
            sum += 15 * 8;
        } else {
            sum += 15 * h;
        }
        return sum;
    }
}

Note the line:

    int sum = (h > 20) ? 20 : 0;

This is ternary operator. Sometimes it is good for conditional assignment in expressions. Thus we initialize the sum variable with 20 when h is greater than 20 and with 0 otherwise. Now we don't use extra if statement in the end of our method.

Now each employee is responsible for calculating pay for itself and there is no need to perform a type check in PayCalculator class - it is dynamically resolved in runtime which code to execute based on parameter type:

public class PayCalculator {

    int pay(Employee e, int hours) {
        return e.calculatePay(hours);
    }

    public static void main(String[] args) {
        Seller seller = new Seller();
        Cook cook = new Cook();
        Chief chief = new Chief();

        PayCalculator calc = new PayCalculator();

        System.out.println("Seller is payed " + calc.pay(seller, 15));
        System.out.println("Cook is payed " + calc.pay(cook, 10));
        System.out.println("Chief is payed " + calc.pay(chief, 22));
    }
}

This is called polymorhism. If this term is new to you, you can read Oracle tutorial in OOP basics: https://docs.oracle.com/javase/tutorial/java/concepts/index.html

Thinking in Java book by Bruce Eckel also has a good explanation of basic OOP concepts.

Upvotes: 6

David Soroko
David Soroko

Reputation: 9086

With Java 8, the language acquired various functional bits which can be used for a functional kind of cleanup. 

    import java.util.EnumMap;
    import java.util.function.IntFunction;

    public class Calc8 {

        public enum Employee {
            SELLER, COOK, CHIEF;
        }

        private final EnumMap<Employee, IntFunction<Integer>> map = new EnumMap<>(Employee.class);

        public Calc8() {
            map.put(Employee.SELLER, h -> {
                int sum = h > 8 ? 20 * (h - 8) + 80 : 10 * h;
                return h > 20 ? sum + 10 : sum;
            });
            map.put(Employee.COOK, h -> {
                int sum = h > 8 ? 30 * (h - 8) + (15 * 8) : 15 * h;
                return h > 20 ? sum + 20 : sum;

            });
            map.put(Employee.CHIEF, h -> {
                int sum = h > 8 ? 66 * (h - 8) + (22 * 8) : 22 * h;
                return h > 20 ? sum + 30 : sum;
            });
        }

        public int evaluate(Employee e, int value) {
            return map.get(e).apply(3);
        }

        public static void main(final String[] args) {
            Calc8 c = new Calc8();
            System.err.println(c.evaluate(Employee.CHIEF, 3) + " should be 66");
        }
    }

Upvotes: -1

Toomtarm Kung
Toomtarm Kung

Reputation: 594

java.util.Map save a lot of if/else, and use Enum as choices for user

public class X {

    public enum Type {
        SELLER, COOK, CHIEF
    }

    private Map<Type, Integer> constantValue1;
    private Map<Type, Integer> constantValue2;
    private Map<Type, Integer> additionalValue;

    public X() {
        initialConstantValue1();
        initialConstantValue2();
        initialAdditionalValue();
    }

    private void initialConstantValue1() {
        constantValue1 = new HashMap<>();
        constantValue1.put(Type.SELLER, 20);
        constantValue1.put(Type.COOK, 30);
        constantValue1.put(Type.CHIEF, 66);
    }

    private void initialConstantValue2() {
        constantValue2 = new HashMap<>();
        constantValue2.put(Type.SELLER, 10);
        constantValue2.put(Type.COOK, 15);
        constantValue2.put(Type.CHIEF, 22);
    }

    private void initialAdditionalValue() {
        additionalValue = new HashMap<>();
        additionalValue.put(Type.SELLER, 10);
        additionalValue.put(Type.COOK, 20);
        additionalValue.put(Type.CHIEF, 30);
    }

    int pay(final Type type, final int h) {
        int sum = 0;
        if (h > 8) {
            sum = constantValue1.get(type) * (h - 8);
            sum += constantValue2.get(type) * 8;
        }
        else {
            sum += constantValue2.get(type) * h;
        }
        if (h > 20) {
            sum += additionalValue.get(type);
        }
        return sum;
    }

}

Upvotes: 0

Related Questions