8
\$\begingroup\$

I have the following code, based on the input to the constructor I need to initialise some values which are wrapped in a simple POJO. These values are constant as you can see.

However I don't like the if/else construct in the set method and had read about NavigableMap as an alternative for holding range values. Any thoughts on how to improve/clean the below or indeed use this construct? I would like to avoid conditional logic where possible.

private Calibrated calibratedValues;

public CalibrationProperties(long odNumber) {
    setCalibratedValues(odNumber);
}

private void setCalibratedValues(long odNumber) {
if (odNumber < 762) {
    this.calibratedValues = new Calibrated(K0, H0, K0_INV, H0_INV, F0);
} else if (odNumber < 866) {
    this.calibratedValues = new Calibrated(K1, H1, K0_INV, H0_INV, F1);
} else if (odNumber < 1011) {
    this.calibratedValues = new Calibrated(K2, H2, K2_INV, H2_INV, F2);
} else {
    this.calibratedValues = new Calibrated(K3, H3, K3_INV, H3_INV, F3);
}

//Two exceptions
if (odNumber == 858){
    this.calibratedValues = new Calibrated(K2, H2, K2_INV, H2_INV, F2);
}
if (odNumber == 1005){
    this.calibratedValues = new Calibrated(K3, H3, K3_INV, H3_INV, F3);
}
}

public Calibrated getCalibratedValues() {
return calibratedValues;
}

/** Convenience class used to hold calibrated values */
static class Calibrated {
private double[] k;
private double[] h;
private double[] kinv;
private double[] hinv;
private double f;

Calibrated(double[] k, double[] h, double[] kinv, double[] hinv, double f) {
    this.k = k;
    this.h = h;
    this.kinv = kinv;
    this.hinv = hinv;
    this.f = f;
}

public double[] getK() {
    return k;
}

public double[] getH() {
    return h;
}
...
\$\endgroup\$
4
\$\begingroup\$

A NavigableMap would just move the current logic into an object. You'd get something like

final static NavigableMap<Long, Calibrated> OD_CALIBRATIONS = new TreeMap();

static {
    OD_CALIBRATIONS.put(Long.MIN_VALUE, new Calibrated(K0, H0, K0_INV, H0_INV, F0));
    OD_CALIBRATIONS.put(762, new Calibrated(K1, H1, K0_INV, H0_INV, F1));
    OD_CALIBRATIONS.put(858, new Calibrated(K2, H2, K2_INV, H2_INV, F2));
    OD_CALIBRATIONS.put(859, new Calibrated(K1, H1, K0_INV, H0_INV, F1));
    OD_CALIBRATIONS.put(866, new Calibrated(K2, H2, K2_INV, H2_INV, F2));
    OD_CALIBRATIONS.put(1005, new Calibrated(K3, H3, K3_INV, H3_INV, F3));
    OD_CALIBRATIONS.put(1006, new Calibrated(K2, H2, K2_INV, H2_INV, F2));
    OD_CALIBRATIONS.put(1011, new Calibrated(K3, H3, K3_INV, H3_INV, F3));
}

private void setCalibratedValues(long odNumber) {
    this.calibratedValues = OD_CALIBRATIONS.floorEntry(odNumber).getValue();
}

Note: I'm not sure about the map key initializations. They get kind of weird since you are mixing less than ranges with single value equalities. You could pull the exceptions out of the NavigableMap and put them in a regular Map instead. That would make the code look something like

final static MAP<Long, Calibrated> OD_EXCEPTION_CALIBRATIONS = new HashMap();
final static NavigableMap<Long, Calibrated> OD_CALIBRATIONS = new TreeMap();

static {
    OD_EXCEPTION_CALIBRATIONS.put(858, new Calibrated(K2, H2, K2_INV, H2_INV, F2));
    OD_EXCEPTION_CALIBRATIONS.put(1005, new Calibrated(K3, H3, K3_INV, H3_INV, F3));

    OD_CALIBRATIONS.put(Long.MIN_VALUE, new Calibrated(K0, H0, K0_INV, H0_INV, F0));
    OD_CALIBRATIONS.put(762, new Calibrated(K1, H1, K0_INV, H0_INV, F1));
    OD_CALIBRATIONS.put(866, new Calibrated(K2, H2, K2_INV, H2_INV, F2));
    OD_CALIBRATIONS.put(1011, new Calibrated(K3, H3, K3_INV, H3_INV, F3));
}

private void setCalibratedValues(long odNumber) {
    if (OD_EXCEPTION_CALIBRATIONS.containsKey(odNumber)) {
        this.calibratedValues = OD_EXCEPTION_CALIBRATIONS.get(odNumber);
    } else {
        this.calibratedValues = OD_CALIBRATIONS.floorEntry(odNumber).getValue();
    }
}

Note that calling setCalibratedValues with both 762 and 763 will give each the same Calibrated object. If that's not what you want, you could give Calibrated a copy constructor and make a new one each time. Also note that this method requires creating all the calibrations at the beginning. This can be wasteful if you only use one per program invocation. Not so bad now, but what if you get more calibration sets?

It would be possible to put the rules into a file or files and then read them into the objects. That would eliminate most of the constants. You'd have to make sure that you read the file by the first time that you needed the values. Perhaps put them into a Singleton object? At worst case then, the Singleton constructor could read the file the first time that it was needed. Or you could prime it earlier.

Some things that I don't like about setCalibratedValues that you didn't mention:

  1. What's an odNumber? Unless odNumber means something, you might as well call it n. If odNumber does mean something, you should tell me so that I know it too. If odNumber isn't a standard name, then I'd much prefer it was written out. Why write out number which should be obvious from the type and leave od obscured?
  2. Similarly, do k, h, k_inv, h_inv, and f have meaning? I'd have trouble editing that code, as I have no idea what each of those means. I sort of think that k and k_inv have something to do with each other, but no idea what.
  3. Also, why are most of them arrays? This should be understandable from the code, and it isn't here.
  4. Your constants are named K0, H1, etc. Why not give these more meaningful names rather than just 0, 1, 2, 3? Of course, this could also argue in favor of moving these out of code entirely. If position is all that separates them, why give them names at all?
  5. You're repeating the exact same calibratedValues and overwriting previous values.

Why not something like

private void setCalibratedValues(long odNumber) {
    if (odNumber < 762) {
        this.calibratedValues = new Calibrated(K0, H0, K0_INV, H0_INV, F0);
    } else if (odNumber < 866 && odNumber != 858) {
        this.calibratedValues = new Calibrated(K1, H1, K0_INV, H0_INV, F1);
    } else if (odNumber < 1011 && odNumber != 1005) {
        this.calibratedValues = new Calibrated(K2, H2, K2_INV, H2_INV, F2);
    } else {
        this.calibratedValues = new Calibrated(K3, H3, K3_INV, H3_INV, F3);
    }
}

This reduces your repeated code. Of course, it won't work as well if the exceptions aren't just off by one in the ranges. Although if 1050 were supposed to get the 0 set, then you could use an || to move that value from the 3 set to the 0.

We might be able to suggest something better if we knew what odNumber, k, h, k_inv, h_inv, and f were and why they are related. For example, if odNumber is the Orson Dines catalog number of a set and the other numbers are dimensions of the parts, you might put those in a database. Then you could read the values from the database as necessary.

\$\endgroup\$
2
\$\begingroup\$

Instead of treating the exceptional values 858 and 1005 this way:

private Calibrated calibratedValues;

public CalibrationProperties(long odNumber) {
    setCalibratedValues(odNumber);
}

private void setCalibratedValues(long odNumber) {
    if (odNumber < 762) {
        this.calibratedValues = ...
    } else if (odNumber < 866) {
        this.calibratedValues = ...
    } else {
        this.calibratedValues = new Calibrated(K3, H3, K3_INV, H3_INV, F3);
    }

    //Two exceptions
    if (odNumber == 858){
        this.calibratedValues = ...
    }
    if (odNumber == 1005){
        this.calibratedValues = ...
    }

It would be better to make the calibratedValues member field final, by reordering the conditions and moving the initialization into the constructor, like this:

private final Calibrated calibratedValues;

public CalibrationProperties(long odNumber) {
    if (odNumber == 858) {
        this.calibratedValues = new Calibrated(K2, H2, K2_INV, H2_INV, F2);
    } else if (odNumber == 1005) {
        this.calibratedValues = new Calibrated(K3, H3, K3_INV, H3_INV, F3);
    } else if (odNumber < 762) {
        this.calibratedValues = new Calibrated(K0, H0, K0_INV, H0_INV, F0);
    } else if (odNumber < 866) {
        this.calibratedValues = new Calibrated(K1, H1, K0_INV, H0_INV, F1);
    } else if (odNumber < 1011) {
        this.calibratedValues = new Calibrated(K2, H2, K2_INV, H2_INV, F2);
    } else {
        this.calibratedValues = new Calibrated(K3, H3, K3_INV, H3_INV, F3);
    }
}

It can be a good investment to create an enum for the conditions to match:

public static enum CalibrationType {
    EQ_858(new EqualMatcher(858), new Calibrated(K2, H2, K2_INV, H2_INV, F2)),
    EQ_1005(new EqualMatcher(1005), new Calibrated(K3, H3, K3_INV, H3_INV, F3)),
    LT_762(new LessThanMatcher(762), new Calibrated(K0, H0, K0_INV, H0_INV, F0)),
    LT_866(new LessThanMatcher(866), new Calibrated(K0, H0, K0_INV, H0_INV, F0)),
    LT_1011(new LessThanMatcher(1011), new Calibrated(K0, H0, K0_INV, H0_INV, F0)),
    DEFAULT(new DefaultMatcher(), new Calibrated(K0, H0, K0_INV, H0_INV, F0))
    ;

    private final CalibrationMatcher matcher;
    private final Calibrated calibrated;

    CalibrationType(CalibrationMatcher matcher, Calibrated calibrated) {
        this.matcher = matcher;
        this.calibrated = calibrated;
    }

    public boolean matches(long odNumber) {
        return matcher.matches(odNumber);
    }
}

Of course, ideally with better names than EQ_858, LT_866, something evoking the meaning of what those conditions represent.

The CalibrationMatcher interface and the various implementations can be something like:

private static interface CalibrationMatcher {
    boolean matches(long odNumber);
}

private static class EqualMatcher implements CalibrationMatcher {
    private final long value;

    private EqualMatcher(long value) {
        this.value = value;
    }

    @Override
    public boolean matches(long odNumber) {
        return odNumber == value;
    }
}

private static class LessThanMatcher implements CalibrationMatcher {
    private final long value;

    private LessThanMatcher(long value) {
        this.value = value;
    }

    @Override
    public boolean matches(long odNumber) {
        return odNumber < value;
    }
}

public static enum CalibrationType {
    eq858(new EqualMatcher(858), new Calibrated(K2, H2, K2_INV, H2_INV, F2)),
    eq1005(new EqualMatcher(1005), new Calibrated(K3, H3, K3_INV, H3_INV, F3)),
    lt762(new LessThanMatcher(762), new Calibrated(K0, H0, K0_INV, H0_INV, F0))
    ;

    private final CalibrationMatcher matcher;
    private final Calibrated calibrated;

    CalibrationType(CalibrationMatcher matcher, Calibrated calibrated) {
        this.matcher = matcher;
        this.calibrated = calibrated;
    }

    public boolean matches(long odNumber) {
        return matcher.matches(odNumber);
    }
}

With the help of these, you could use a for-loop in the constructor on the values of the enum:

public CalibrationProperties(long odNumber) {
    Calibrated calibrated = null;
    for (CalibrationType type : CalibrationType.values()) {
        if (type.matches(odNumber)) {
            calibrated = type.calibrated;
            break;
        }
    }
    this.calibratedValues = calibrated;
}
\$\endgroup\$
1
\$\begingroup\$

Considering that two of the six cases are special cases, it's not worth the trouble to try to generalize the switching logic. Rather, I'd just rewrite it using a chained conditional expression.

private void setCalibratedValues(long odNumber) {
    this.calibratedValues =
        (odNumber <   762) ? new Calibrated(K0, H0, K0_INV, H0_INV, F0) :
        (odNumber ==  858) ? new Calibrated(K2, H2, K2_INV, H2_INV, F2) :
        (odNumber <   866) ? new Calibrated(K1, H1, K0_INV, H0_INV, F1) :
        (odNumber == 1005) ? new Calibrated(K3, H3, K3_INV, H3_INV, F3) :
        (odNumber <  1011) ? new Calibrated(K2, H2, K2_INV, H2_INV, F2) :
                             new Calibrated(K3, H3, K3_INV, H3_INV, F3);
}

I've also arranged the thresholds in increasing order and lined everything up horizontally for easy visual comparison.

\$\endgroup\$
0
\$\begingroup\$

You can use a Builder/Factory pattern. This will hide the if/then logic (which is not too bad if it is really a static ruleset. And it looks pretty irregular, so it would be more ugly to work with ranges.) and it will also allow to make the values static:

Sample with an inline factory method:

private final Calibrated calibratedValues;

private CalibrationProperties(Calibrated calibrated) {
    this.calibratedValues = calibrated;
}

public static CalibratedProperties getInstance(long odNumber) {
    if (odNumber == 858) {
       return new CalibratedProperties(new Calibrated(K2, H2, K2_INV, H2_INV, F2));
    }
    if (odNumber < 762) {
        return new CalibratedProperties(new Calibrated(K0, H0, K0_INV, H0_INV, F0));
    } else if (odNumber < 866) {
       ...
}

I also pulled the exception to the front.

The only other option is to have it read from a external text file. This allows adjustment by non-developers and makes the code clean.

\$\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.