9

(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?

  • 4
    The purpose of an interface is to establish a pattern of connection, not to represent behavior. That duty falls to the methods in the implementation. – Robert Harvey Feb 1 '17 at 17:15
  • As to your implementation, there's an awful lot of boilerplate (code that doesn't actually do anything, other than provide structure). Are you sure you need all of it? – Robert Harvey Feb 1 '17 at 17:36
  • Even interface's methods are only 'getters' it does not means you cannot implements 'setters' on the implementation (if all have setter,s you can still use an abstract) – рüффп Feb 3 '17 at 19:27
4

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.

  • 1
    I'm not sure I completely agree with having HolidayDiscount and RentalDiscount implementing IDiscountCalculator, because they're not discount calculators. The calculation is done in that other method getMaxApplicableLengthDiscount. HolidayDiscount and RentalDiscount are discounts. A discount is not calculated, is a type of applicable discount what is calculated, or just selected among a number of discounts. – user3748908 Feb 3 '17 at 11:29
  • 1
    Maybe the Interface should be called IDiscountable. Whatever classes need to implement can. If the holiday/rental discount classes are just DTO/POCO and store the result rather than calculating that is fine. – Jon Raynor Feb 3 '17 at 14:59
2

To answer your question: you cannot conclude a code smell if an interface has only getters.

An example for a fuzzy metric for code smells are bloated interfaces with many methods (doesn't if getters or not). They tend to violate the interface seggregation principle.

On the semantic level the single responsibility principle should not be violated as well. I tends to be violated if you see several different issues handled in the interface contract that are not one subject. This is sometimes difficult to identify and you need some experience to identify it.

On the other side you should be aware if your interface defines setters. That's because setters are likely to change state, that is their job. The question to ask here: Does the object behind the interface makes a transition from a valid state into another valid state. But that is not mainly a problem of the interface but the implementation of the interface contract of the implementing object.

The only concern I have with your approach is that you operate on OR-Mappings. In this little case this is not that a problem. Technically it is ok. But I would not operate on OR-Mapped objects. They can have too many different states you surely not consider in operations done on them...

OR-Mapped objects may be (presuppose JPA) transient, persistent detached unchanged, persistent attached unchanged, persistent detached changed, persistent attached changed... if you use bean validition on those objects you further cannot see if the object was checked or not. After all you can identify more than 10 different states the OR-Mapped object can have. Do all your operations handle these states properly?

This is certainly no problem if your interface defines the contract and the implementation follows it. But for OR-mapped objects I have reasonable doubt the can fullfill such a contract.

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.