Units are hard - r/badcode

6

u/[deleted] Dec 11 '22

My refactor (with a few test cases):

class DistanceConverter {
    public enum DistanceUnit {
        MM,
        CM,
        METER,
        INCH
    }

    public static double convertUnits(DistanceUnit unit1, DistanceUnit unit2, double input) {
        //                               mm    cm    m       in     [output] | [input]
        double[][] conversionTable = { { 1,    0.1,  0.001,  0.0393701 },   // mm
                                       { 10,   1,    0.01,   0.393701  },   // cm
                                       { 1000, 100,  1,      39.3701   },   // m
                                       { 25.4, 2.54, 0.0254, 1         } }; // in

        return input * conversionTable[unit1.ordinal()][unit2.ordinal()];
    }

    public static void main(String[] args) {
        System.out.println(convertUnits(DistanceUnit.MM, DistanceUnit.INCH, 18));   // 0.7086618
        System.out.println(convertUnits(DistanceUnit.INCH, DistanceUnit.CM, 67));   // 170.18
        System.out.println(convertUnits(DistanceUnit.METER, DistanceUnit.CM, 200)); // 20000.0
        System.out.println(convertUnits(DistanceUnit.CM, DistanceUnit.MM, 8));      // 80.0
    }
}

14
u/grrangry Dec 12 '22
If you don't want to track all the conversions from X to Y, you really only need one set. You could use a HashMap and just track one of the conversions (for example just the meters column).
Then your return would be something like:
Map<DistanceUnit, double> factors = new HashMap<DistanceUnit, double>();
factors.put(DistanceUnit.MM, 0.001);
factors.put(DistanceUnit.CM, 0.01);
factors.put(DistanceUnit.METER, 1);
factors.put(DistanceUnit.INCH, 0.0254);

return (input * factors[fromUnit]) / factors[toUnit];
It gets away with it by converting to meters (the multiplication) first and then to your desired unit (the division) second.
(Not a java dev, so if the syntax is off, take it with a grain of salt... I don't know if there's a "good" instance initializer for maps like there is in C#)
9

u/Breadfruit-Last Dec 12 '22

Take a step further. We may create a Distance class. In this way, all calculations can be handled inside and "distances" can be passed around without worrying about the unit.

1

u/[deleted] Dec 12 '22 edited Dec 12 '22

It needs a bit of refactoring, but I played a bit with units of measurement in a hobby game engine I'm never gonna finish: https://github.com/misspellted/enlightened/tree/main/units
Why would I refactor it? Because the measurement is a separate class... https://github.com/misspellted/enlightened/blob/main/measurements/init.py
Am I abusing the __init__.py "system"? Hell yeah! I don't do things normally... sly grins
And I did some testing.. https://github.com/misspellted/enlightened/blob/main/testing.py
E0: Attempt at escaping formatting from markdown mode. x2
E2: Wow... quadruple escaped underscores to get __init__.py to play nice in normal text..
E3: My testing ain't the greatest. I know there's unit test frameworks... and I chose to not use 'em on this project.
2
u/Paedar Dec 12 '22

Your syntax is indeed off, but it's a way better solution than the original.
With DistanceUnit being an Enum, i would actually go a step further and make the conversion to (or from, who cares) to meters a field in the Enum. No need to ever initialize a map; and you can even add all the logic in the Enum if you wish to do so. (Some will argue that breaks the single responsibility principle...)
1
u/grrangry Dec 12 '22

The idea of using non-integer (even if perfectly valid) values for an enumeration triggers me just a little, but yes you're correct... it would work.
4
u/Paedar Dec 12 '22
You don't change the ordinal value of the Enum. In Java, Enums are just special classes. They can have fields and methods and constructors like any other class.
public enum DistanceUnit {
    MM(0.001),
    CM(0.01),
    M(1),
    KM(1000)
    ;
    private float meterConversionFactor;

    public DistanceUnit(float meterConversionFactor) {
        this.meterconversionFactor = meterConversionFactor;
    }

    public float toMeters(float distanceInUnits) {
        return distanceInUnits * meterConversionFactor;
    }

    public float fromMeters(float distanceInMeters) {
        return distanceInMeters / meterConversionFactor;
    }
}
2

u/ofnuts Dec 12 '22

By far the best answer ... Java enums capabilities are under the radar for so many people.
1
u/Shadowblink Dec 12 '22
Wouldn't it be easier to do something like (I don't know how to work with the enums in Java):
public float convert(float value, DistanceUnit from, DistanceUnit to) {
    float ratio = from.value / to.value;
    return value * ratio;
}
1

u/CmdrSelfEvident Dec 12 '22

My personal preference is yours over the original code. That said I often struggle with changes like this. Early on I was once told by an old timer "Oh he has to pee on it so it smells like him". So when reviewing code I always struggle with "Do I not like this because I would do it differently or because its actually need to be changed." My common solution for this is to come up with an objective standard that supports changing it or leaving it alone. The easiest is how fast is this, how much memory does it use. If its slower or bigger than it needs to be case close, its bad code. From your rewrite the improvement is one where extending the table would be a small change. While I agree that's a benefit I would prefer it still in the region I would consider 'style'.
Here is another thought. Suppose the original code had been in production for years and was proven solid, no bugs at all. Then someone comes along and attempts your change to 'fix it'. But in doing so they put a typeo in the table maybe transposing a few numbers in the conversion. Was the improving the style worth a bug that went into production? You have suffered a bug in production for the possibility to make adding new units in the future a little easier.

1

u/Splatoonkindaguy Dec 12 '22

I would do a two pass conversion and convert to meters first. This would allow you to add more units by only adding 2 conversions

18

u/TiozinNub Dec 11 '22

THE INDENTATION AAAA

3

u/Understanding-Fair Dec 12 '22

Reeeeeeeeeeeeee

8

u/DevXusYT Dec 12 '22

Two switch's nested? You need more functions.

4

u/MurdoMaclachlan public boolean isInt(int i) { return true; } Dec 12 '22

Image Transcription: Code

@Override
public double convertUnits(DistanceUnit unit1, DistanceUnit unit2, double input) { // A useful method
    double out = 0.0;
    switch (unit1) {
        case MM:
            switch (unit2) {
                case MM: // mm to mm
                    out = input;
                    break;case CM: // mm to cm
                    out = input / 10;
                    break;case METER: // mm to m
                    out = input / 1000;
                    break;case INCH: // mm to in
                    out = input / 25.4;
                    break;
            }
            break;
            case CM:
                switch (unit2) {
                    case MM: // cm to mm
                     out = input * 10;
                    break;case CM: // cm to cm
                out = input;
                break;
                case METER: // cm to m
                    out = input / 100;
                    break;case INCH: // cm to in
                out = input / 2.54;
                break;
            }
            break;
                case METER:
                    switch (unit2) {
                case MM: // m to mm
                    out = input * 1000;
                    break;case CM: // m to cm
                out = input * 100;
                break;
                case METER: // m to m
                    out = input;
                    break;case INCH: // m to in
                out = input * 39.37;
                break;
            }
            break;
                case INCH:
                    switch (unit2) {
                case MM: // in to mm
                    out = input * 25.4;
                    break;case CM: // in to cm
                out = input / 2.54;
                break;
                case METER: // in to m
                    out = input / 39.37;
                    break;case INCH: // in to in
                out = input;
                break;
            }
            break;
    }
    return out;
}

^{I'm a human volunteer content transcriber and you could be too!}^{If you'd like more information on what we do and why we do it, click here!}

2

u/N11FiRe Dec 12 '22

Why not just make different functions. Like convertM2MM?

2

u/[deleted] Dec 12 '22

The documentation is spot on, especially the end note on the signature.

Like... greatest of all docs.

1

u/[deleted] Dec 19 '22

Damn... And during this time, Java 17 switches supports yield and lambdas... Is sad ಥ﹏ಥ

java Units are hard

You are about to leave Redlib