2
\$\begingroup\$

I m working on calisthenics objects and I'm actually having some troubles while trying to follow the calisthenic objects rules.

In fact, I m stuck with the one telling that we have to wrap all the primitives inside of objects.

Context

I'm having a Euro object that looks like :

public class Euro {

    private double amount;

    public Euro(double amount) {
        this.amount = amount;
    }

    @Override
    public boolean equals(Object obj) {
        Euro other = (Euro)obj;
        return other.amount == amount;
    }

    // Problems come here
    public Euro multiply(double priceValue) {
        Euro newAmount = new Euro(amount * priceValue);
        return newAmount;
    }

    public Euro add(Euro valueToAdd) {
        Euro newPrice = new Euro(amount + valueToAdd.amount);
        return newPrice;
    }
}

And I m having a Discount class. In fact, we can apply Discount on product, the the amount we obtain is a Euro :

public class Discount {

    private double value;

    public Discount(double theValue) {
        value = theValue;
    }

    public Euro apply(Euro price) {
        return price.multiply(value);
    }
}

Problem

My problems appear on the multiply method of the Euro object.

In fact, currently, I m accepting a double as parameter, and it doesn't follow the rule of wrapping all primitives in objects... Moreover, a "double" in a business context doesn't mean anything at all. I m a little bit stuck with this example and don't know how to bypass this.

Moreover, I don't want to use getter / setter on this implementation...

Any suggestions ?

\$\endgroup\$
2
  • 1
    \$\begingroup\$ Sylvain Boisse already summed up the most important points I was about to say, so the addition comes as a small comment: I noticed, that you use double to represent a monetary value. This is generally a bad idea, as floating point types do not have an exact representation of numbers in all cases. For money, use some kind of integer/long type (in cents obviously) or BigDecimal. \$\endgroup\$
    – mtj
    Jan 26, 2017 at 6:27
  • \$\begingroup\$ @mtj Good comment. This also allows good handling on rounding: there are legal requirements on rounding the cents (if the seller applies a 30% rabate on 7.99€ etc.), which a dedicated class would allow. \$\endgroup\$
    – MrBrushy
    Jan 27, 2017 at 9:40

1 Answer 1

3
\$\begingroup\$

That cannot work with this Object hierarchy. You have a contradiction in your katas, that requires some messy stuff.

Here's an extract of the katas of your previous similar Question (emphasis mine):

One level of indentation per method
Don’t use the ELSE keyword
Wrap all primitives and Strings
First class collections
One dot per line
Don’t abbreviate
Keep all entities small (50 lines)
No classes with more than two instance variables
No getters/setters/properties

You're dealing with two double values of distinct meaning (a discount rate and a cost). Because of kata Wrap all primitives, you wrap them both in two, distinct object.

Now you need these primitives (the doubles) to interact (multiply). No Object can access both values, since the kata No getters prevents one from accessing the other's internal values!


A Solution

You've successfully made two primitives interact elsewhere. It was in the Euro class:

public Euro add(Euro valueToAdd) {
    Euro newPrice = new Euro(amount + valueToAdd.amount);
    return newPrice;
}

A primitive wrapper is able to make interactions with a primitive wrapper of the same type, or with a subclass of it. Then that is what you should do: all double wrappers that will want to interact must extend a common double wrapper. (Note 1: I didn't spot any rule against subclassing, yet you did not subclass anything. Note 2: does subclassing bypass kata No classes with more than two instance variables? Don't need it, just curious).

So here's the code:

MonetaryQuantity.java (Edit: Changed the name, fits better) is a parent class to all stuff that will ever interact with money etc.)

public class MonetaryQuantity {
    protected double value; // Can be accessed by any Money-related class
    public MonetaryQuantity(double theValue) {
        value = theValue;
    }
}

Euro.java:

public class Euro extends MonetaryQuantity{
    public Euro(double amount) {
        super(amount);
    }
    public Euro apply(Discount discount) {
        Euro newAmount = discount.applyOn(this);
        return newAmount;
    }
    public Euro add(Euro valueToAdd) {
        Euro newPrice = new Euro(value + valueToAdd.value);
        return newPrice;
    }
}

Discount.java:

public class Discount extends MonetaryQuantity{
    public Discount(double theValue) {
        super(theValue);
    }
    public Euro applyOn(Euro price) {
        // Discount can access Euro.value because both handle money (similar business sphere)
        return new Euro(price.value * value); 
    }
}

Notes on style

Why do you apply a Euro on a Discount? Usually, we apply a Discount on a Price. I prefer price.apply(discount). I kept discount.applyOn(price).

Naming value on Euro is a bit clumsy. It may be a cost, or a price, those things are different. Maybe two subclasses?

Naming value on Discount is just bad. It should be a rate. The Object might even be called a DiscountRate, because you could have an AbsoluteDiscount (-10€). Discount would be an interface with a Euro applyOn(Euro price) method. Note I moved the implementation of the discount from the Euro class to the Discount class for just that reason. Euro just forwards it to Discount.

Note: with these Katas, I cannot extract an add and a multiply method into the wrapper class (although I would really like it!) because it would have to return the proper Wrapper sub-class (not the primitive result). It would only work with C-style result-passing in parameters AND require a setValue():

public class MonetaryQuantity {
    public <T extends MonetaryQuantity> T add(MonetaryQuantity valueToAdd, T returnValue) {
        double newValue = value + valueToAdd.value;
        returnValue.set(newValue); // This is against a kata!
        return newValue;
    }
}

This is all quite frustrating.


The equals() method

It's broken everywhere. It fails with ClassCastExceptions, NullPointerExceptions etc. Make instanceof checks and handle the mismatch cases properly.

\$\endgroup\$
6
  • \$\begingroup\$ Thank you for this great answer, I m going to study your thoughts and try to apply them in my context. This kata is just killing me x) \$\endgroup\$
    – mfrachet
    Jan 25, 2017 at 21:04
  • 1
    \$\begingroup\$ One or two remarks, DoubleWrapper doesn't make a perfect sense in a business context. Moreover using a Money parent class to deal with Euro and a Discount seems to be weird, since Discount is not a cost, but a rate \$\endgroup\$
    – mfrachet
    Jan 26, 2017 at 8:04
  • 1
    \$\begingroup\$ I know, I couldn't find anything at the time. Now I think of it, it could be a MonetaryQuantity ? This encompasses a cost, a price, or a discount. I may edit my answer if you like it. \$\endgroup\$
    – MrBrushy
    Jan 26, 2017 at 9:01
  • \$\begingroup\$ How can the private value member of MonetaryQuantity be accessed in subclasses? \$\endgroup\$ Mar 14, 2017 at 14:43
  • \$\begingroup\$ Renaming DoubleWrapper to MonetaryQuantity doesn't change the fact that it is a wrapper around double, and its subclasses have nothing in common other than the fact that they share a floating-point representation. It seems like a cumbersome workaround to satisfy a pointless rule banning primitives. \$\endgroup\$ Mar 14, 2017 at 14:47

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.