The following problem has been written using TDD. Please review the following:

1. Unit tests


2. OOP


3. Design Principles (SOLID)


4. Cleanliness and naming


5. Anything that could make the program better.

Problem Statement:
Taxi Meter Company has to develop an app for their business. The app calculates the bill based on the car type and the distance covered. They have 3 different types of car and they charge the base price for the first 5 miles as
follows:

1. Mini - $100


2. Light - $200


3. Compact - $300

After they cross 5 miles, the customer will be charged at 10$/mile
for the next 10 miles. Once they cross 15 miles, they will be charged at 20$/mile.
If the total distance covered is greater than 100 miles, they will be charged at 10$/mile for every mile (the total distance). Base price will be added by default in addition to the charges for the miles. Please refer sample input and output.

Sample Input:

• Mini, 10 Miles


• Light, 5 Miles


• Mini, 20 Miles


• Compact, 120 Miles

Sample Output:

• 150


• 200


• 300


• 1500

Test cases (Unit tests:)

public class TaxiMeterAppSpec {

    @Test

    public void givenTheDistanceCoveredForTheFirst5KmsAndTheCarTypeAsMiniTheBaseFareShouldBe100() {

        Meter meter = new Meter(5);

        CarType car = new Mini(meter);

        TaxiMeterApp taxiMeter = new TaxiMeterApp(car);

        assertEquals(100, taxiMeter.showPrice());

    }



    @Test

    public void givenTheDistanceCoveredForTheFirst5KmsAndTheCarTypeAsLightTheBaseFareShouldBe200() {

        Meter meter = new Meter(5);

        CarType car = new Light(meter);

        TaxiMeterApp taxiMeter = new TaxiMeterApp(car);

        assertEquals(200, taxiMeter.showPrice());

    }



    @Test

    public void givenTheDistanceCoveredForTheFirst5KmsAndTheCarTypeAsCompactTheBaseFareShouldBe300() {

        Meter meter = new Meter(5);

        CarType car = new Compact(meter);

        TaxiMeterApp taxiMeter = new TaxiMeterApp(car);

        assertEquals(300, taxiMeter.showPrice());

    }



    @Test

    public void ifTheDistanceIsMoreThan100KmsAndTheCarTypeIsCompactTheTotalFareShouldBe1800() {

        Meter meter = new Meter(150);

        CarType car = new Compact(meter);

        TaxiMeterApp taxiMeter = new TaxiMeterApp(car);

        assertEquals(1800, taxiMeter.showPrice());

    }



    @Test

    public void ifTheDistanceIsMoreThan100KmsAndTheCarTypeIsMiniTheTotalFareShouldBe1600() {

        Meter meter = new Meter(150);

        CarType car = new Mini(meter);

        TaxiMeterApp taxiMeter = new TaxiMeterApp(car);

        assertEquals(1600, taxiMeter.showPrice());

    }



    @Test

    public void ifTheDistanceIsBetween16And100KmsAndTheCarTypeIsMiniTheTotalFareShouldBe900() {

        Meter meter = new Meter(50);

        CarType car = new Mini(meter);

        TaxiMeterApp taxiMeter = new TaxiMeterApp(car);

        assertEquals(900, taxiMeter.showPrice());

    }



    @Test

    public void ifTheDistanceIsBetween16And100KmsAndTheCarTypeIsCompact_TheTotalFareShouldBe1100() {

        Meter meter = new Meter(50);

        CarType car = new Compact(meter);

        TaxiMeterApp taxiMeter = new TaxiMeterApp(car);

        assertEquals(1100, taxiMeter.showPrice());

    }



    @Test

    public void ifTheDistanceIsBetween6And15KmsAndTheCarTypeIsCompact_TheTotalFareShouldBe390() {

        Meter meter = new Meter(14);

        CarType car = new Compact(meter);

        TaxiMeterApp taxiMeter = new TaxiMeterApp(car);

        assertEquals(390, taxiMeter.showPrice());

    }



}

TaxiMeterApp: Shows the final price based on the type of car

public class TaxiMeterApp {



    private CarType carType;



    public TaxiMeterApp(CarType carType) {

        this.carType = carType;

    }



    public int showPrice() {

        Bill bill = new Bill(this.carType);

        return bill.calculate();

    }



}

CarType: Abstract class for Car type

public abstract class CarType {



    protected int baseCharge;



    private Meter meter;



    public CarType(Meter meter) {

        this.meter = meter;

    }



    public int getBaseCharge() {

        return baseCharge;

    }



    public Meter getMeter() {

        return meter;

    }



}

Mini: Type of car

public class Mini extends CarType {



    public Mini(Meter meter) {

        super(meter);

        this.baseCharge = 100;

    }



}

Light: Type of car

public class Light extends CarType {



    public Light(Meter meter) {

        super(meter);

        this.baseCharge = 200;

    }



}

Compact: Type of car

public class Compact extends CarType {



    public Compact(Meter meter) {

        super(meter);

        this.baseCharge = 300;

    }

}

Meter:

public class Meter {



    private int distance;



    public int getDistance() {

        return distance;

    }



    public Meter(int distance) {

        this.distance = distance;

    }

}

Bill: Class that generates the bill

public class Bill {



    private CarType carType;



    public Bill(CarType carType) {

        this.carType = carType;

    }



    public int calculate() {

        return this.carType.getBaseCharge() + calculatePriceBasedOn(this.carType.getMeter());

    }



    private int calculatePriceBasedOn(Meter meter) {

        int distance = meter.getDistance();

        if(distance > 100) {

            return distance * 10;

        } else if(distance <=100 && distance > 15) {

            return (distance - 15) * 20 + 100;

        } else if(distance > 5){

            return (distance - 5) * 10;

        }

        return 0;

    }

}

Code structure

I find it odd that a CarType would know the distance it traveled. This class is supposed to model the type of the car, and this does not depend on the distance: a CarType is the same whether you traveled 100 or 1000 miles with it. What really depend on the distance traveled is the price; therefore, I'd refactor the method showPrice to take as parameter the meter:

public int showPrice(Meter meter) {

    Bill bill = new Bill(carType, meter);

    return bill.calculate();

}

The idea is that when constructing a TaxiMeterApp, you don't know in advance the distance the person is going to travel, only the car type they chose. And note that Bill constructor would now take 2 parameters: the car type and the miles traveled, which is all it needs to derive the price.

A second point is the usage of an abstract class CarType with multiple implementations. If you intend in the future to let users of the API create new CarTypes, this is a good solution; if not, consider having an enumeration, which will be simpler:

public enum CarType {



    MINI(100), LIGHT(200), COMPACT(300);



    private final int baseCharge;



    CarType(int baseCharge) {

        this.baseCharge = baseCharge;

    }



    public int getBaseCharge() {

        return baseCharge;

    }

}

The tests

There is a lot of repeat in your tests. They are all structured the same way, with just data changing:

@Test

public void givenTheDistanceCoveredForTheFirst5KmsAndTheCarTypeAsCompactTheBaseFareShouldBe300() {

    Meter meter = new Meter(5);

    CarType car = new Compact(meter);

    TaxiMeterApp taxiMeter = new TaxiMeterApp(car);

    assertEquals(300, taxiMeter.showPrice());

}

The only change with subsequent tests is that the type of the car, the distance and the expected result are different. Meaning the only changes are only data-changes. The same algorithm is tested on different data and it is asserted whether it behaves correctly or not. This leads to code duplication: if tomorrow you refactor the main algorithm, all of your tests will break and you'll need to revise each one to update them. Indeed, if you implement some of the changes discussed previously, you'll need to do just that.

The first possible solution is to refactor the structure of the test inside one method. You can create a method that takes the distance and the car type, and that would return the price:

private int priceForCarTypeAndMeter(int distance, CarType carType) {

    Meter meter = new Meter(distance);

    TaxiMeterApp taxiMeter = new TaxiMeterApp(carType);

    return taxiMeter.showPrice(meter);

}

and then, all of tests are refactored to:

@Test

public void givenTheDistanceCoveredForTheFirst5KmsAndTheCarTypeAsMiniTheBaseFareShouldBe100() {

    assertEquals(100, priceForCarTypeAndMeter(5, new Mini()));

}

without any code duplication as to how the price is actually derived from the distance and the car type.

Another approach in this case, is to have data-driven tests, which consists of a single test method, but which is invoked on multiple data. If you're using JUnit 4 or below, that can be done with parameterized tests and starting with JUnit 5, you can use dynamic tests.

Meter

I think this class totally useless since you only wrap an int (for nothing ?)
Don't you think it's a bit redundant to write Meter meter = new Meter(5) instead of int meter = 5 ?

I would remove it.

CarType

As Tunaki said, I won't couple the distance into the CarType, this has nothing to do with a CarType.

Don't you have the feeling when you compare Mini, Light and Compact that there's a lot of redundancy ? From my point of view, it's screaming for an enum since the only difference between all of that is the value of baseCharge.

I would refactor all of that into the following:

public enum Car {

    MINI(100), LIGHT(200), COMPACT(300);



    private final int baseCharge;



    Car(int baseCharge) {

        this.baseCharge = baseCharge;

    }



    public int getBaseCharge() {

        return baseCharge;

    }

}

Bill

What does a Bill need in order to compute a price ? A base charge and a distance. These two values must be provided in the bill constructor. The bill doesn't have to know about the Car. Then, the implementation becomes (note that I have made this class immutable):

public final class Bill {

    private final int baseCharge;

    private final int meter;



    public Bill(int baseCharge, int meter) {

        this.baseCharge = baseCharge;

        this.meter = meter;

    }



    public int calculate() {

        return baseCharge + calculatePriceBasedOn(meter);

    }



    private int calculatePriceBasedOn(int distance) {

        if (distance > 100) {

            return distance * 10;

        } else if (distance <= 100 && distance > 15) {

            return (distance - 15) * 20 + 100;

        } else if (distance > 5) {

            return (distance - 5) * 10;

        }

        return 0;

    }

}

TaxiMeterApp

This class also doesn't need to depend on Car, the only useful information is the amount of the base charge.

One now takes meter as a parameter since the distance is obviously not known when starting the taxi meter (the base charge is, however). I also removed the App from the class name because I think it has no value and only bring visual noise.

public final class TaxiMeter {

    private final int baseCharge;



    public TaxiMeter(int baseCharge) {

        this.baseCharge = baseCharge;

    }



    public int showPrice(int meter) {

        Bill bill = new Bill(baseCharge, meter);

        return bill.calculate();

    }

}

TaxiMeterAppSpec

I find your tests hard to understand for 2 reasons:

• Different naming conventions in method names


• A mixin between given, then and when
  

This is the kind of test I would write (I picked a convention for the names, you can choose another since it stays coherent). Given the previous refactors, the test implementations are more expressive now:

public class TaxiMeterSpec {

    @Test

    public void itShouldCost100WhenAMiniDrives5() {

        TaxiMeter taxiMeter = new TaxiMeter(Car.MINI.getBaseCharge());



        int price = taxiMeter.showPrice(5);



        assertEquals(100, price);

    }



    @Test

    public void itShouldCost200WhenALightDrives5() {

        TaxiMeter taxiMeter = new TaxiMeter(Car.LIGHT.getBaseCharge());



        int price = taxiMeter.showPrice(5);



        assertEquals(200, price);

    }



    //and so on...

}

The biggest thing I see missing here, which would clean up the code considerably, is the addition of a TaxiTrip class:

public class TaxiTrip

{

    public TaxiTrip(CarType carType) {

        // ...

    }



    public Bill calculateBill(Meter meter) {

        // create Bill and calculate charges

    }

}

This allows you to decouple the car type, meter and bill.

Your Class model does not look right to me.

You should create a new class when there is new behavior. Your CarType extensions only differ in Properties. Therefore, creating classes is not justified.

The better way was to chenge the abstract CarType - class to a regular class and all the different cars to enum values:

public enum CarType {

   Type(100),Light(200),Compact(300);



   private final int baseCharge;

   CarType(int baseCharge){

      this.baseCharge = baseCharge;

   }



   public int getBaseCharge(){

      return baseCharge;

   }

}



class Car {

  private final CarType carType;

  private final Meter meter;



  public CarType(Meter meter, CarType carType) {

     this.meter = meter;

     this.carType = carType;

  }

  // ...

}

public abstract class CarType {

    protected int baseCharge;

    private Meter meter;

    public CarType(Meter meter) {

        this.meter = meter;

    }

This part has two smells:

• Visibility of baseCharge: properties of a class should have least possible visibility. In your case there is no need to raise the visibility since there is a getter for it in this class already.



• 
  

  You use 2 different techniques to set the property values. This is called an odd ball solution. You'd better passed both values as constructor parameters:

  
  
  

  public abstract class CarType {
  
      private int baseCharge;
  
      private Meter meter;
  
      public CarType(Meter meter, int baseCharge) {
  
          this.meter = meter;
  
          this.baseCharge = baseCharge;
  
      }