Constraining data in idiomatic Java

Question

Playing with a simple Java Point class where I would like to constrain the X and Y values to be doubles that must be in the range -10 to 10, inclusive. I've written some code, but it's been years since I've written java and I would like to know if this is how it would be written in modern Java:

public class Point {
    private double x;
    private double y;

    public Point(double x, double y) {
        constrain("x", x);
        constrain("y", y);
        this.x = x;
        this.y = y;
    }

    // is there a cleaner/shorter way of handling this, such as a direct way of declaring a
    // subtype of double that I could use in method signatures?
    protected static void constrain(String name, double val) {
        if ( val < -10 || val > 10 ) {
            throw new IllegalArgumentException(name + " must be between -10 and 10");
        }
    }

    public double getX() { return x; }

    public void setX(double x) {
        constrain("x", x);
        this.x = x;
    }

    public double getY() { return y; }

    public void setY(double y) {
        constrain("y", y);
        this.y = y;
    }

    @Override
    public String toString() {
        return ("[" + x + "," + y + "]"); 
    }
}

Answer 1

Remove the magic numbers for the constrained values and give them constants instead:

private static final double MAX_VAL = 10;
private static final double MIN_VAL = -10;

These constants should also be printed in the exception statement instead of hard-coding the values. Otherwise, if you just change the constants, the code will give inaccurate results.

Regarding the class name, consider renaming it to something like Constrained2DPoint. Points can also have a 'z' if they're used in 3D space, so the name Point may be ambiguous in either case.

Answer 2

@Jamal's answer already mentioned the most important points, I think there's little left to add.

The method name constrain is too generic, and doesn't tell much about what kind of constraining it does. Since the method cannot be overridden (because it's static), and it does a specific kind of constraint checking, I'd suggest a more specific name, for example checkBounds or validateBounds or ensureWithinBounds.

The fact that the method is protected static suggests that it's intended to be used by other classes too. That can be confusing. Unless there is a specific reason to do it this way, I suggest to change the access modifier to private, and make it non-static.

As per your questions:

// is there a cleaner/shorter way of handling this, such as a direct way of declaring a
// subtype of double that I could use in method signatures?

Although you cannot create a sub-type of Double (because it's a final class), you could create a custom class that would perform the bound checking in its constructor, and use that class in the method signatures in your Point class. However, your constrain method has an additional feature of reporting the variable name ("x" or "y") that failed the validation. So to preserve the same behavior, the custom bounded double type would need to be constructed with a name parameter in additional to the double value, which would be inconvenient for users. I think it's better to leave the bound checking in this class.

Answer 3

Rather than reduce code duplication by implementing a constrain() method, I think it is more important that your constructor reuse the .setX(x) and .setY(y) codepaths.

In addition, I would find it slightly disconcerting, when looking at the stack trace, that the IllegalArgumentException was thrown in a method called constrain() that I didn't call. An IllegalArgumentException thrown by your library's code looks at first glance like it's a bug in your library's code, not in my caller.

In this case, I'd live with a bit of code duplication in the validation. (It's possible to factor out the validation logic but still have .setX() and .setY() throw the exceptions themselves, but the weirdness outweighs the gains, I think.)

public class Point {
    private double x;
    private double y;

    public Point(double x, double y) {
        this.setX(x);
        this.setY(y);
    }

    public double getX() { return x; }
    public double getY() { return y; }

    public void setX(double x) {
        if (x < -10 || x > 10) {
            throw new IllegalArgumentException("x must be between -10 and 10");
        }
        this.x = x;
    }


    public void setY(double y) {
        if (y < -10 || y > 10) {
            throw new IllegalArgumentException("y must be between -10 and 10");
        }
        this.y = y;
    }

    @Override
    public String toString() {
        return ("[" + x + "," + y + "]"); 
    }
}

I should mention in closing that in many instances, a Point class like this should be immutable, so there wouldn't be a public .setX() and .setY() at all.