Java – Is an interface with only getters a code smell

code-qualityjavaobject-orientedobject-oriented-design

(I've seen this question, but the first answer goes about auto properties more than about design, and the second one says hide the data storage code from the consumer, which I'm not sure is what I want/my code does, so I'd like to hear some other opinion)

I have two very similar entities, HolidayDiscount and RentalDiscount, that represent length discounts as 'if it lasts at least numberOfDays, a percent discount is applicable'. The tables have fks to different parent entities, and are used in different places, but where they're used, there is a common logic to get the maximum applicable discount. For instance, a HolidayOffer has a number of HolidayDiscounts, and when calculating its cost, we need to figure out the applicable discount. Same for rentals and RentalDiscounts.

Since the logic is the same, I want to keep it in a single place. That's what the following method, predicate and comparator do:

Optional<LengthDiscount> getMaxApplicableLengthDiscount(List<LengthDiscount> discounts, int daysToStay) {
    if (discounts.isEmpty()) {
        return Optional.empty();
    }
    return discounts.stream()
            .filter(new DiscountIsApplicablePredicate(daysToStay))
            .max(new DiscountMinDaysComparator());
}

public class DiscountIsApplicablePredicate implements Predicate<LengthDiscount> {

    private final long daysToStay;

    public DiscountIsApplicablePredicate(long daysToStay) {
        this.daysToStay = daysToStay;
    }

    @Override
    public boolean test(LengthDiscount discount) {
        return daysToStay >= discount.getNumberOfDays();
    }
}

public class DiscountMinDaysComparator implements Comparator<LengthDiscount> {

    @Override
    public int compare(LengthDiscount d1, LengthDiscount d2) {
        return d1.getNumberOfDays().compareTo(d2.getNumberOfDays());
    }
}

Since the only information needed are the number of days, I end up with an interface as

public interface LengthDiscount {

    Integer getNumberOfDays();
}

And the two entities

@Entity
@Table(name = "holidayDiscounts")
@Setter
public class HolidayDiscount implements LengthDiscount {

    private BigInteger percent;

    private Integer numberOfDays;

    public BigInteger getPercent() {
        return percent;
    }

    @Override
    public Integer getNumberOfDays() {
        return numberOfDays;
    }

}

@Entity
@Table(name = "rentalDiscounts")
@Setter
public class RentalDiscount implements LengthDiscount {

    private BigInteger percent;

    private Integer numberOfDays;

    public BigInteger getPercent() {
        return percent;
    }

    @Override
    public Integer getNumberOfDays() {
        return numberOfDays;
    }
}

The interface has a single getter method that the two entities implement, which of course works, but I doubt it's a good design. It doesn't represent any behavior, given holding a value is not a property. This is a rather simple case, I have a couple more of similar, more complex cases (with 3-4 getters).

My question is, is this a bad design? What is a better approach?

Best Answer

I would say your design is a little misguided.

One potential solution would be to create an interface called IDiscountCalculator.

decimal CalculateDiscount();

Now any class that needs to provide a discount would implement this interface. If the discount uses number of days, so be it, interface doesn't really care as that is the implementation details.

The number of days probably belongs in some sort of abstract base class if all discount classes need this data member. If it is class specific, then just declare a property that can be accessed publicly or privately depending on the need.

Related Topic