Electricity billing system for home in OOP

Question

I recently appeared for interview for code-pairing round and I was asked following question: Electricity Billing System for home. The corresponding power usage of home appliances (Fan, AC, Fridge, Light) was given:

Appliance | Per hour unit consumption
Fan 4
Light 2
AC 10
Fridge 8

The slab chart was given as:

Slab 1 - 0 to 1000 unit: INR Rs 20 per unit
Slab 2 - 1001 to 3000 unit: INR Rs 30 per unit
Slab 3 - 3001 to 6000 unit: INR Rs 40 per unit
Slab 4 - 6001 and above unit: INR Rs 50 per unit

Input:

Appliance | CountOfAppliance | TotalUasgePerDayInHours
Fan 2 4
Light 1 4
AC 1 12
Fridge 1 5

Output:

200000 INR
Units Per Day : (2*4*4 + 1*4*2 +1*12*10 + 1*5*8) = 200
unitsPerMonth = 6000
totalBill = 1000*20 + 2000*30 + 3000*30 + 3000*50 = 200000

I had modeled the code using "Factory Design Pattern" for appliance. But my Code for Price Slab was something the interviewer was not happy with the hardcoding. Though I modified it by using Constants file for slabs but the interviewer was still expecting something better. Kindly let me know how it can be improved.

IElectricComponent

public interface IElectricComponent {
    public enum Unit{
           FAN(4), LIGHT(2), AC(10) , FRIDGE(8);
           private int value;
           private Unit(int value) {
                this.value = value;
           }
           public int getValue(){
            return value;
           }
       }
    public int claculateUnitForSingleDay();
}

Fan

public class Fan implements IElectricComponent{

    private int noOfComponents;
    private int perDayUsage;
    public Fan(int noOfComponents, int perDayUsage){
        this.noOfComponents=noOfComponents;
        this.perDayUsage=perDayUsage;               
    }

    public int claculateUnitForSingleDay(){
        return noOfComponents*perDayUsage*Unit.FAN.getValue();      
    }   
}

The same way for Fridge, Light and AC

Factory: ApplianceFactory

public class ApplianceFactory {

        public static IElectricComponent getInstance(String appliance, int countOfAppliance ,int perDayUsage ){

            switch(appliance){

                case ApplianceConstants.FAN:
                    return new Fan(countOfAppliance,perDayUsage);

                case ApplianceConstants.LIGHT:
                    return new Light(countOfAppliance,perDayUsage);

                case ApplianceConstants.AC:
                    return new AC(countOfAppliance,perDayUsage);

                case ApplianceConstants.FRIDGE:
                    return new Fridge(countOfAppliance,perDayUsage) ;

                default :
                    return new IElectricComponent() {

                        @Override
                        public int claculateUnitForSingleDay() {
                            // TODO Auto-generated method stub
                            return countOfAppliance*perDayUsage;
                        }
                    };
            }

        }
    }

Constants:

public interface ApplianceConstants {

    public String FAN = "Fan";
    public String LIGHT = "Light";
    public String AC = "AC";
    public String FRIDGE = "Fridge";

    public int Slab1 = 20;
    public int Slab2 = 30;
    public int Slab3 = 40;
    public int Slab4 = 50;
}

PriceSlab:

public class PriceSlab {

    HashMap<String,Integer> slabs = new HashMap<>();

    public int calculateBill(int units){

        slabs.put("A", ApplianceConstants.Slab1);
        slabs.put("B", ApplianceConstants.Slab2);
        slabs.put("C", ApplianceConstants.Slab3);
        slabs.put("D", ApplianceConstants.Slab4);

        return calculateBillTotal(units);
    }

    private int calculateBillTotal(int units) {

        int total = 0;
        if(units <= 1000){
            total = units*slabs.get("A") ;
        }
        else if (units <= 3000){
            total = 1000*slabs.get("A") + (units-1000)*slabs.get("B");
        }
        else if (units <= 6000){
            total = 1000*slabs.get("A") + 2000*slabs.get("B") +(units-3000)*slabs.get("C");
        }
        else{
            total = 1000*slabs.get("A") + 2000*slabs.get("B") + 3000*slabs.get("D")
                +(units-6000)*slabs.get("C");
        }
        return total;
    }
}

Main class:

public class BillGenerator {

    public static void main(String[] args) {

        Scanner scn = new Scanner(System.in);
        ApplianceFactory factory = new ApplianceFactory();
        int inputOfAppliance = scn.nextInt();
        String appliance="";
        int countOfAppliance;
        int perDayUsage;
        int unitsPerDay=0;
        for(int t=0 ; t<inputOfAppliance ;t ++){
            appliance = scn.next();
            countOfAppliance = scn.nextInt();
            perDayUsage = scn.nextInt();
            IElectricComponent electricComponent = factory.getInstance(appliance, countOfAppliance, perDayUsage);
            unitsPerDay += electricComponent.claculateUnitForSingleDay();
        }
        System.out.println("unitsPerDay = "+unitsPerDay);
        int unitsPerMonth = unitsPerDay * 30;
        System.out.println("unitsPerMonth = "+unitsPerMonth);

        PriceSlab slab= new PriceSlab();
        int totalBill = slab.calculateBill(unitsPerMonth);
        System.out.println("totalBill = "+totalBill);
    }

}

Please review my code and let me know if anything could be improved.

Answer 1

Attention to Detail

What seem like trivial mistakes can be interpreted as a lack of attention to detail. So whilst your interface has a simple typo claculateUnitForSingleDay vs calculateUnitForSingleDay, it is noticeable. You don't want this slipping into production code, particularly not on an interface or an API object because you can find yourself stuck with the embarrassing typo for years.

Similarly, leaving things like 'TODO' in your code sets off a red flag to reviewers.

IElectricalComponent

You've created an electrical component interface and from what I can tell a concrete implementation for each of the possible components. In the bigger picture, this might make sense, however as it stands, this seems like overkill. The component basically does one thing, calculate the total number of units used by a given number of components of that type in a day. The calculate has three inputs number of that component, how much each is used per day in hours, cost in units per hour. You've modelled the first two as properties and the last one as a type. I don't see why it's different, so would have modelled that as part of the electrical component calculation. If I do this, then I can remove all of your implementations and replace your Factory with:

public static IElectricComponent getInstance(String appliance, 
                                             int countOfAppliance ,
                                             int perDayUsage ){
    int unitsPerHour = IElectricComponent.Unit.valueOf(appliance.toUpperCase()).getValue();

    return () -> countOfAppliance * perDayUsage * unitsPerHour;
}

PriceSlabs

Looking at your solution to the price slab calculation, again, it seems quite rigid. Whilst you've used constants for the different slab costs, you've hard coded knowledge about the number of units and created a separate branch of code for each. This is error prone and indeed, it looks to me like you have a bug in your final condition:

total = 1000*slabs.get("A") + 2000*slabs.get("B") + 3000*slabs.get("D")
            +(units-6000)*slabs.get("C");

I'm pretty sure that it should be 3000*slabs.get("C") + (units-6000)*slabs.get("D").

I think it would be better to use some kind of table based approach. This allows you to be more flexible to changes + reduces the number of branches that need to be considered. So, for example it could be replaced with something like this:

public class PriceSlab {

    static class Slab{
        private final int unitCost;
        private final int slabSize;

        Slab(int slabSize, int unitCost) {
            this.slabSize = slabSize;
            this.unitCost = unitCost;
        }

    }

    static final List<Slab> PRICE_SLABS = asList(new Slab(1000, Slab1),
            new Slab(2000, Slab2),
            new Slab(3000, Slab3),
            new Slab(0, Slab4 )
            );

    public int calculateBill(int units) {
        int total = 0;

        for(Slab currentSlab : PRICE_SLABS) {
            int unitsInSlab = currentSlab.slabSize > units || currentSlab.slabSize == 0 
                ? units : currentSlab.slabSize;

            total += unitsInSlab * currentSlab.unitCost;

            units -= unitsInSlab;
        }

        return total;
    }
}

You can see how the PRICE_SLABS list could be easily extracted and supplied (from a database/config file) as a parameter to the calculate method, so that it becomes calculateBill(units, priceSlabs).

Error Checking

In my alternative suggestions, I've ignored error checking. Adding some in helps to demonstrate that you've considered possible error conditions and how you'd handle them. So, for example, is it ok to have more than 24 as the total usage per day in hours? If the user mistypes Fan do you really want the application to silently default to a different calculation?

default :
     return new IElectricComponent() {
         @Override
         public int claculateUnitForSingleDay() {
             // TODO Auto-generated method stub
             return countOfAppliance*perDayUsage;
         }
     };

Consider the impact this would have on peoples bills if a new expensive component was introduced and the application wasn't updated before the new billing cycle.

Answer 2

public class Fan implements IElectricComponent{

private int noOfComponents;
private int perDayUsage;
public Fan(int noOfComponents, int perDayUsage){
    this.noOfComponents=noOfComponents;
    this.perDayUsage=perDayUsage;               
}

public int claculateUnitForSingleDay(){
    return noOfComponents*perDayUsage*Unit.FAN.getValue();      
}   

}