Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Foreign Units must be handled with no Surprises #209

Open
andi-huber opened this issue Apr 5, 2019 · 5 comments
Open

Foreign Units must be handled with no Surprises #209

andi-huber opened this issue Apr 5, 2019 · 5 comments

Comments

@andi-huber
Copy link
Member

andi-huber commented Apr 5, 2019

Several code fragments assume that any Unit is an (or a subtype of) AbstractUnit.

  1. This might result in ClassCastExceptions, when instead we should throw a proper and more descriptive UnsupportedOperationException.

  2. Some code-paths are simply ignored, when a Unit is not of type AbstractUnit. Its critical to make sure, we properly throw an UnsupportedOperationException instead, to prevent users from running into incorrect calculations. (Calculations must fail instead!)

@andi-huber andi-huber self-assigned this Apr 5, 2019
@andi-huber
Copy link
Member Author

waiting on #204 to be resolved/merged

@keilw
Copy link
Member

keilw commented Apr 10, 2019

There are a few cases which assume AbstractUnit, a few more the RI extension interface ComparableUnit. I would rather throw an IllegalArgumentException or MeasurementException because it is specific to the Measurement RI. UnsupportedOperationException seems only suitable if you want to mark the whole method as unsupported by this implementation.

@andi-huber
Copy link
Member Author

For example when looking at ProductUnit

    @Override
    public UnitConverter getSystemConverter() {
        UnitConverter converter = AbstractConverter.IDENTITY;
        for (Element e : elements) {
            if (e.unit instanceof AbstractUnit) {
                UnitConverter cvtr = ((AbstractUnit<?>) e.unit).getSystemConverter();
                if (!(cvtr.isLinear()))
                    throw new UnsupportedOperationException(e.unit + " is non-linear, cannot convert");
                if (e.root != 1)
                    throw new UnsupportedOperationException(e.unit + " holds a base unit with fractional exponent");
                int pow = e.pow;
                if (pow < 0) { // Negative power.
                    pow = -pow;
                    cvtr = cvtr.inverse();
                }
                for (int j = 0; j < pow; j++) {
                    converter = converter.concatenate(cvtr);
                }
            }
        }
        return converter;
    }

If the ProductUnit contains an element that is not an instance of AbstractUnit, we still return a converter, but we should instead fail, I guess! I believe we have multiple such code fragments that do not behave well, when 'foreign' Units are used.

@andi-huber
Copy link
Member Author

andi-huber commented Apr 11, 2019

As an alternative to failing with exceptions, we could instead within RI lookout for places that allow to replace/narrow method arguments Unit<Q> with AbstractUnit<Q>, such that it is clear, what type the RI expects for proper operation.

@keilw
Copy link
Member

keilw commented Apr 13, 2019

This may not work in every case, but here the logical solution would be offering getSystemConverter() to the API element Unit instead.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants