3
\$\begingroup\$

I want to perform mathematical operations on a range of values, where zeros should be ignored. This means that multiplication by zero does not result in zero but returns the first operand and likewise (\$5x0=5\$), division by zero does not throw an ArithmeticException but returns the first operand (\$5/0=5\$).

I would like to avoid testing for zero every time I need to make a calculation and would prefer that the calculation logic takes care of it.

My question is about the best way of approaching this problem. I will be using BigDecimal and thought of a simple implementation of the decorator pattern that adds functionality to the multiply and divide methods.

With this in mind, I developed the following code snippet:

public class BigDecimalWrapper extends BigDecimal {


public BigDecimalWrapper multiply(BigDecimal augend){
    if(augend.compareTo(BigDecimal.ZERO) != 0){
        return new BigDecimalWrapper(super.multiply(augend).doubleValue());
    } else {
        return this;
    }
}

public BigDecimalWrapper divide(BigDecimal augend){
    if(augend.compareTo(BigDecimal.ZERO) != 0){
        return new BigDecimalWrapper(super.divide(augend).doubleValue());
    } else {
        return this;
    }
}

public BigDecimalWrapper add(BigDecimal augend){
    return new BigDecimalWrapper(super.add(augend).doubleValue());
}

public BigDecimalWrapper subtract(BigDecimal augend){
    return new BigDecimalWrapper(super.subtract(augend).doubleValue());
}

// Some code omitted for brevity.
}

I expect to use it like this:

@Test
public void shouldMultiply() {
    BigDecimalWrapper v = new BigDecimalWrapper(10).multiply(BigDecimal.ZERO);
    assertThat(v).isEqualTo(new BigDecimal(10));
}

@Test
public void shouldDivide() {
    BigDecimalWrapper v = new BigDecimalWrapper(10).divide(BigDecimal.ZERO);
    assertThat(v).isEqualTo(new BigDecimal(10));
}

One of the problems that this is intended to resolve is in the calculation of quantity where it is not known if it is required to calculate length, area or volume. All that is known at the time of calculation are the dimensions. We may have only length, or length and width or height, width and length, therefor height may be 0 and while width and length are given. I would hope to be able to calculate with just one line of code as follows:

new BigDecimalWrapper(width).multiply(height).multiply(length);

Therefore, if height is zero, it will effectively ignore it and multiply width and length.

There are other similar situations where it will be used in this manner and is therefore worth the effort developing a solution for it.

Is this the best way to approach this problem? What alternative ways should I consider?

\$\endgroup\$
2
  • 2
    \$\begingroup\$ I am just curious why you need this kind of arithmetic. It would give some unexpected results, e.g 2*2 - 2*2 = 4 - 4 = 0, but 2*(2-2) = 2*0 = 2. \$\endgroup\$ – Martin R Jul 25 '15 at 14:07
  • \$\begingroup\$ I agree that this will cause problems if used as you suggest, however the usage will only ever be simply and only as shown in the question. I have added an edit to the question explaining a scenario in which it will be used. Thank you for your feedback. \$\endgroup\$ – Alex Jul 25 '15 at 17:01
5
\$\begingroup\$

There are big problems with your approach.

Cloning BigDecimal with .doubleValue() is broken

Cloning a BigDecimal bd using new BigDecimal(bd.doubleValue()) doesn't work in most practical cases. For example, try this code:

BigDecimal bd = BigDecimal.valueOf(0.02d);
BigDecimal clone = new BigDecimal(bd.doubleValue());
System.out.println(bd);
System.out.println(clone);
System.out.println(bd.equals(clone));

On my computer the above code prints:

0.02
0.0200000000000000004163336342344337026588618755340576171875
false

The equality test fails for as simple values as 2d. It passes for a simple 2, but that's not very reassuring.

As such, your current implementation of cloning doubles is effectively broken, for example this test case fails:

@Test
public void multiply_2d_with_3d() {
    BigDecimalWrapper v = new BigDecimalWrapper(2d).multiply(BigDecimal.valueOf(3d);
    assertThat(v).isEqualTo(new BigDecimal(2d).multiply(BigDecimal.valueOf(3d));
}

You can fix it by replacing .doubleValue() with .toString(), the unit test will pass.

Cloning BigDecimal is a strange thing to do

BigDecimal instances are immutable, so you can safely pass around copies. Creating new instances is completely unnecessary. Your current approach forces you to clone. Since cloning is non-sense, this indicates a code smell, and that a better solution is probably available.

Composition over inheritance

A better approach would be to avoid the strange cloning, by making the decorated object a final field in your wrapper class. Add a constructor that takes a BigDecimal, and call that one instead of super.

By the way, putting the decorated object into a field of the decorator class is the common technique when implementing this pattern (composition instead of inheritance). It makes it possible to nest multiple decorators in arbitrary order. Inheritance is a very tight coupling, composition gives you much more flexibility.

For example something like this:

class BigDecimalWrapper {

    private final BigDecimal bigDecimal;

    public BigDecimalWrapper(double num) {
        bigDecimal = new BigDecimal(num);
    }

    public BigDecimalWrapper(BigDecimal bigDecimal) {
        this.bigDecimal = bigDecimal;
    }

    public BigDecimalWrapper multiply(BigDecimal augend) {
        if (augend.compareTo(BigDecimal.ZERO) != 0) {
            return new BigDecimalWrapper(bigDecimal.multiply(augend));
        } else {
            return this;
        }
    }

    public BigDecimal bigDecimalValue() {
        return bigDecimal;
    }
}

Since the class no longer extends BigDecimal, I added a bigDecimalValue() method to get access to the wrapped object. The unit tests can be modified to work with this, and they pass:

@Test
public void shouldMultiply() {
    BigDecimalWrapper v = new BigDecimalWrapper(10).multiply(BigDecimal.ZERO);
    assertThat(v.bigDecimalValue()).isEqualTo(new BigDecimal(10));
}

@Test
public void multiply_2d_with_3d() {
    BigDecimalWrapper v = new BigDecimalWrapper(2d).multiply(BigDecimal.valueOf(3d);
    assertThat(v.bigDecimalValue()).isEqualTo(new BigDecimal(2d).multiply(BigDecimal.valueOf(3d));
}

You might not like that fact the BigDecimalWrapper is no longer a BigDecimal. But maybe that's a good thing. With your implementation, if somebody ever tries to use a BigDecimalWrapper instance as a BigDecimal, he might be surprised that multiplication with zero doesn't work as expected from a BigDecimal. For this reason I think it's a good thing for BigDecimalWrapper to not pretend to be a BigDecimal.

If, however, you really want BigDecimalWrapper to be a BigDecimal, then the most sensible solution is to keep your original implementation, with doubleValue() replaced with toString().

\$\endgroup\$
2
  • \$\begingroup\$ I understand that there are some fundamental problems with the code given in the above. I want to use the code snippet to start a conversation regarding the best way to deal with the problem of zeros in a multiplication/division. I am looking for a way to do calculations that may or may not involve zeros where if there are zeros they don't cause the code to error or return zero, and to avoid using lots of conditional logic in the main body of code by encapsulating it in the BigDecimalWrapper. I am sure this problem must have been solved before, but a google search did not yield results. Thanks \$\endgroup\$ – Alex Jul 25 '15 at 17:10
  • \$\begingroup\$ Thanks for your code review. I have implemented the changes you have advised and checked in a complete implementation to the following github repository: github.com/atheedom/Calculator \$\endgroup\$ – Alex Jul 26 '15 at 8:01
0
\$\begingroup\$

multiply()'s code:

if (augend.compareTo(BigDecimal.ZERO) != 0) {
     return new BigDecimalWrapper(bigDecimal.multiply(augend));
} else {
     return this;
}

can be simplified to:

return augend.compareTo(BigDecimal.ZERO) != 0
    ? new BigDecimalWrapper(bigDecimal.multiply(augend))
    : this;

The same applies to divide().

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