5
\$\begingroup\$

We have an exercise to design a simple library, which can calculate some length with different unit, e.g.

2m*2 + 20cm/2 - 5mm

and the result can be any of:

4.095m
409.5cm
4095mm

I want to design it in Object-oriented, and have a solution like this:

public class Length {

    private final double value;
    private final Unit unit;

    public Length(double value, Unit unit) {
        this.value = value;
        this.unit = unit;
    }

    @Override
    public boolean equals(Object obj) {
        Length length = (Length) obj;
        return this.unit.toMM(this.value) == length.unit.toMM(length.value);
    }

    public Length add(Length added) {
        return new Length(this.unit.toMM(this.value) + added.unit.toMM(added.value), Unit.mm);
    }

    public Length subtract(Length another) {
        return new Length(this.unit.toMM(this.value) - another.unit.toMM(another.value), Unit.mm);
    }

}

enum Unit {
    m(1000), cm(10), mm(1);
    private final int rate;

    Unit(int rate) {
        this.rate = rate;
    }

    public double toMM(double value) {
        return rate * value;
    }
}

It works, but I have several questions:

  1. Is there anything wrong from the OO point of view?
  2. Unit has a method toMM which is bind to a concrete type of unit mm(millimeter), I don't feel good about it, is there any way to improve it? Or it's just acceptable since we have to choose one?
  3. Length has chosen mm for all kinds of unit, is it good? I'm looking for a way to avoid it, but not found

These questions are mostly based on this possible requirement change: don't want to support mm anymore. Most of the code has to be modified in this case.


Update: from this article, seems it's not a good idea to use enum here for different units?

\$\endgroup\$
  • \$\begingroup\$ There should be a bug when comparing two double here with ==, but now let's ignore it \$\endgroup\$ – Freewind Dec 2 '15 at 14:42
  • \$\begingroup\$ Your example at the start is a little confusing. 2 meters squared + 20 cm squared - 5 mm. Not sure what you would expect this to be other than an error? \$\endgroup\$ – Paddy Dec 2 '15 at 22:07
  • \$\begingroup\$ @Paddy maybe that's meant to be read as "2 meters times 2, i.e. 4 meters" plus "20 centimeters divided by 2, i.e. 10 centimeters" minus 5 millimeters? \$\endgroup\$ – h.j.k. Dec 3 '15 at 15:52
6
\$\begingroup\$

An alternative approach would be to say that a length is measured in SI units (meters).

Also, if performance is not a concern, using a BigDecimal instead of a double would avoid potential rounding errors.

It could look like:

public class Length {
  private final BigDecimal meters;
  public Length(BigDecimal meters) { ... }
  public Length(BigDecimal value, Unit unit) { /*convert to m*/}

  public BigDecimal getLength(Unit unit) { /*convert to unit*/ }
  public BigDecimal getLength() { /*return length in SI, i.e. meters*/ }

  public int hashCode() { return meters.hashCode(); }
  public boolean equals(Object o) {
    /*usual checks */
    return meters.compareTo(other.meters) == 0;
  }
  //other methods
}

This also avoids having to arbitrarily choose a "base" unit (mm in your example).

Finally I would suggest defining an interface for Unit:

public interface Unit {
  BigDecimal toMeters(BigDecimal length);
  //and possibly additional helper methods:
  default BigDecimal convert(BigDecimal length, Unit sourceUnit) { ... }
}

Then your enum would be:

public enum StandardUnit implements Unit {
  MM("0.001"); //etc.

  private final BigDecimal multiplier;
  StandardUnit(String mulltiplier) { this.multiplier = new BigDecimal(multiplier); }
  public BigDecimal toMeters(BigDecimal length) { return length.multiply(multiplier); }
}

This will allow a more flexible design where you can add units by implementing the interface or use the provided standard units.

| improve this answer | |
\$\endgroup\$
3
\$\begingroup\$

Firstly, you must implement hashcode if you implement equals. Read up here.

I don't think that equals should convert to mm. I think it should compare units && value. It's equals, not equivalent, if you want to have that behaviour, give it another name (suggestion isSameLength). Or you could convert to mm in the constructor and only store mm internally, and maybe the toString applies intelligence about how best to display it and possibly with an overload that takes the desired display unit.

You say to ignore the double comparison, but just a quick note on that, you could use a decimal type such as BigDecimal internally, because its how it is actually being represented to the user and the maths won't leave you with any rounding errors that later make comparison hard.

Another approach could be to drop the enum and have classes for each type and a base type Length.

e.g: You could have types for each Unit of measure, and return a MM instance from your toMM. Then move toMM to each subtype, this suggestion will actually remove need for your enum:

(constructors omitted, but they take just one BigDecimal)

public abstract class Length {
    public abstract Millimeter toMM();

    public boolean isSameLength(Length other){
        return toMM().equals(other.toMM()); //Must implement equals on Millimeter class
    }
}

public class Meter extends Length {
    private final BigDecimal meters;

    @Override
    public Millimeter toMM() {
        return new Millimeter(meters * 1000);
    }
}

public class Centimeter extends Length {
    private final BigDecimal centimeters;

    @Override
    public Millimeter toMM() {
        return new Millimeter(centimeters * 100);
    }
}

public class Millimeter extends Length {
    private final BigDecimal millimeters;

    @Override
    public Millimeter toMM() {
        return this;
    }
}

The nice thing about this is it can be extended without modifying existing code. If I want to start using Furlongs:

public class Furlong extends Length {
    private final BigDecimal furlongs;

    @Override
    public Millimeter toMM() {
        return new Millimeter(furlongs * 201168);
    }
}

And I do not have to modify an enum or any other file.

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ Thanks, @weston. A similar question: abstract base class Length now have a dependency of Millimeter in your design now. So it still will be a big change if "don't want to support millimeter anymore" happens \$\endgroup\$ – Freewind Dec 2 '15 at 14:55
  • \$\begingroup\$ Another question is about this It's equals, not equivalent. What's the difference between them, and why it's better to use another method for my equals? \$\endgroup\$ – Freewind Dec 2 '15 at 14:57
  • 1
    \$\begingroup\$ I entirely disagree with the argument re: equals(). The class name is Length. In all ways 1000 mm is equal to 1m and is equal to 3.28084 ft. A length is simply a distance, and the distances are equal. The coin argument is equally disingenuous. A dollar is a dollar no matter how you add it up. BUT, this only applies to the concept of MONEY. If you want a coin collector's app, then you're not talking about Money at all. You're talking about discrete objects, with a value different than the face value. \$\endgroup\$ – Chris Cudmore Dec 2 '15 at 16:30
  • 1
    \$\begingroup\$ No, that's not correct at all. The CLIENT defines equality. If the client is an engineer, or a carpenter, then the only thing they care about is that 1 m = 1000 mm. In fact, I doubt there is anyone in the world who would want a program/class/framework that states that 1 m !=1000mm . It is factually incorrect. Just because the coding decision checks off something on your smell list, doesn't mean it's bad. Limburger stinks, and delivers exactly as expected. A smell is a warning, not an error. \$\endgroup\$ – Chris Cudmore Dec 2 '15 at 17:45
  • 1
    \$\begingroup\$ But the overriding principle is the Principle of Least Surprise/Astonishment. I would be astonished if 1 m != 1000mm. And as a user of the class called length, the least surprising thing would be that two lengths that are equal, would be programmatically equal. Don't be beholden to guidelines, they are not rules. Certainly the EASIEST way is not to override it. But easiest isn't always best. But yes, you should always ask the question "Do I need to?". But the utility of equals over some other non-overridden method is something I want. \$\endgroup\$ – Chris Cudmore Dec 2 '15 at 19:16
1
\$\begingroup\$

With minimal changes of the original code:

public class Length {
    private final double value;
    private final Unit unit;

    public Length(double value, Unit unit) {
        this.value = value;
        this.unit = unit;
    }

    @Override
    public boolean equals(Object obj) {
        Length length = (Length) obj;
        return this.unit.normalize(this.value) == length.unit.normalize(length.value);
    }

    public Length add(Length added) {
        return new Length(this.unit.normalize(this.value) + added.unit.normalize(added.value), Unit.base);
    }

    public Length subtract(Length another) {
        return new Length(this.unit.normalize(this.value) - another.unit.normalize(another.value), Unit.base);
    }

}

enum Unit {
    m(1000), cm(10), mm(1);
    private final int rate;

    Unit(int rate) {
        this.rate = rate;
    }

    public double normalize(double value) {
        return rate * value;
    }

    static Unit base = mm;
}

If mm is no longer supported, only Unit.base must be modified.

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ I don't like "rate". It's a number, that when multiplied by this unit, gives the equal measurement in a base unit (in this case mm). CONVERT_TO_BASE_UNIT_MULTIPLIER is explicit, and good, but a bit wordy. Any other ideas? \$\endgroup\$ – Chris Cudmore Dec 2 '15 at 19:22
1
\$\begingroup\$
public Length add(Length added) {
    return new Length(this.unit.toMM(this.value)
                        + added.unit.toMM(added.value), Unit.mm);
}

public Length subtract(Length another) {
    return new Length(this.unit.toMM(this.value)
                        - another.unit.toMM(another.value), Unit.mm);
}

From an API perspective, you may want to consider if it's suitable to 'default' Unit as Unit.mm for all the results of these operations. In other words, when the user is adding 2 meters to 2 meters, is the result better represented as 4 meters, or 4000 millimeters? How about adding 50 centimeters to 2 meters? Should it default to 2.5 meters, or 250 centimeters?

What's nice about your code is that your classes are properly modeled as immutable ones - to nitpick, Length should be made final too if you do not wish to extend it. Just remember that the comparison in equals(Object) must check for the same type as well, i.e. catch these as false assertions instead of having the JVM throw a ClassCastException.

| improve this answer | |
\$\endgroup\$

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.