5
\$\begingroup\$

While answering this question, I wanted to use a generic sum of Collection function. Java doesn't have a native method, so I wrote one. There are a couple things I don't like about it though, so I wrote a more specific method for my answer.

public static <T extends Number, U extends Collection<T>> BigInteger sum(U numbers) {
    BigInteger sum = BigInteger.ZERO;

    for ( T number : numbers ) {
        sum = sum.add(new BigInteger(number.toString()));
    }

    return sum;
}

The first problem of course is that this wouldn't work for all Number types. In particular, Float, Double, and BigDecimal are not guaranteed to fit in an integer type. That's easy enough to fix:

public static <T extends Number, U extends Collection<T>> BigDecimal sum(U numbers) {
    BigDecimal sum = BigDecimal.ZERO;

    for ( T number : numbers ) {
        sum = sum.add(new BigDecimal(number.toString()));
    }

    return sum;
}

Now the problem is that it switches from an integer type to a decimal type. So we have to convert it back if we really preferred an integer type.

I'm also not happy with converting to and from a String to convert it to a BigDecimal. They're all numeric types. Why add this intermediate step? Unfortunately, String is the only interoperable type. All the Number types have a toString method, and BigDecimal has a constructor from a String.

Overall, this may be more hacky than useful. Does anyone have an alternative version that works better? Or should I stick with writing a custom method each time the issue arises? Or am I being too hard on this version?

\$\endgroup\$
  • 1
    \$\begingroup\$ If you are able to use Java 8, then you can quite easily use streams to do the summing. There is still no single generic method though that works on all Collection<Number>. \$\endgroup\$ – Simon Forsberg Jan 21 '15 at 18:16
  • \$\begingroup\$ @SimonAndréForsberg While I'm not using Java 8 at the moment, I'm not opposed to a Java 8 specific solution. I'm most interested in the comparative arguments: "I think this is better because..." I'm especially interested in arguments that acknowledge weaknesses, e.g. "Even though this requires a custom method call each time, I think this is better because..." \$\endgroup\$ – Brythan Jan 21 '15 at 18:58
6
\$\begingroup\$

Choosing between Long/Integer and Float/Decimal

I'd let the user choose. Not because that's the best way, but because I don't see any other way.

Number itself is not reflective on the presence of decimal digits. It would need an interface Integer and interface Decimal or something like that in order to deal with this in a single signature. However, the API is not designed this way.

So, if both is needed, I'd have a sumInt and a sumFloat method, or maybe sumInteger and sumDecimal. (I prefer Float over Decimal because Decimal is simply wrong because it means 1/10ths and that's just the wrong name. BigDecimal is the wrong name.)

Simpler usage of Generics

The type parameter U is redundant. You could directly go for this signature:

public static <T extends Number> BigDecimal sum(Collection<T> numbers)

And given that inside you actually don't need T, Number is sufficient, you could go for this signature:

public static BigDecimal sum(Collection<? extends Number> numbers)

Going via String is a bit "far away" from numbers, and unnecessary. You could use Number.longValue() in case of BigInteger and Number.doubleValue() in case of BigDecimal.

Then the code would look like this:

public static BigInteger sumInt(final Collection<? extends Number> numbers) {
    BigInteger sum = BigInteger.ZERO;
    for (final Number number : numbers)
        sum = sum.add(new BigInteger(number.longValue());
    return sum;
}

public static BigDecimal sumFloat(final Collection<? extends Number> numbers) {
    BigDecimal sum = BigDecimal.ZERO;
    for (final Number number : numbers)
        sum = sum.add(new BigDecimal(number.doubleValue());
    return sum;
}

How it looks like with Java 8 Streams

With Java 8 Streams, the code could look like this:

public static BigDecimal sumFloat(final Collection<? extends Number>  numbers) {
    return numbers
            .parallelStream()
            .mapToDouble(Number::doubleValue)
            .mapToObj(BigDecimal::new)
            .reduce(BigDecimal.ZERO, BigDecimal::add);
}

LSP Violation

However, all these implementations violate the LSP - Liskov Substitution Principle. They will not work correctly for all potential Number types. If the Collection numbers contains BigDecimal or BigInteger objects which exceed the range expressible in double or long, the functions would fail to produce the correct result.

Given the signature, which suggests that any Number would do, and given the fact that class BigInteger extends Number and class BigDecimal extends Number, and given the fact that the signature uses BigInteger resp. BigDecimal as a return value, that would be a pretty unexpected behavior.

The following helper function can fix that problem for the known classes BigDecimal and BigInteger:

public static BigInteger toBigInteger(final Number number) {
    return
        Number instanceof BigInteger ? (BigInteger) number :
        Number instanceof BigDecimal ? ((BigDecimal) number).toBigInteger() :
        new BigInteger(number.longValue());
}

However, if someone creates another subclass of Number which exceeds the range of long, there still would be a problem. This problem could only be solved by fixing the API: class Number should have toBigInteger() and toBigDecimal() methods, which in Number, could be implemented with corresponding defaults. Because the root cause for the LSP violation is not in your code, not in my code, but in the API - in the way how class Number actually is defined.

Using the helper function, the code could look like this:

public class Numbers {

    public static BigInteger sumInt(final Collection<? extends Number> numbers) {
        BigInteger sum = BigInteger.ZERO;
        for (final Number number : numbers)
            sum = sum.add(toBigInteger(number);
        return sum;
    }

    public static BigDecimal sumFloat(final Collection<? extends Number>  numbers) {
        return numbers
                .parallelStream()
                .map(Numbers::toBigDecimal)
                .reduce(BigDecimal.ZERO, BigDecimal::add);
    }


    public static BigInteger toBigInteger(final Number number) {
        return
            Number instanceof BigInteger ? (BigInteger) number :
            Number instanceof BigDecimal ? ((BigDecimal) number).toBigInteger() :
            new BigInteger(number.longValue());
    }
    // ...
}
\$\endgroup\$
  • 1
    \$\begingroup\$ @belka Thanks for spotting that faux pas and fixing it. \$\endgroup\$ – Christian Hujer Jul 11 '18 at 13:47
2
\$\begingroup\$

It's inevitable that you will have to compromise between flexibility and performance:

  • A flexible solution that handles all kinds of numbers and overflows will have to work in BigDecimal
  • An efficient solution will have to work with the biggest type you need to support and not bigger. For example, if you want to add up 100 small integers fast, you'll certainly want to do direct additions with sum += item, and avoid instance creations at every step.

Unfortunately in Java there is no magic bullet that would cover both of these at the same time, so you will have to decide the balance that is acceptable in your use case.

If you don't need to support both of these cases at the same time, then you can provide two implementations to satisfy each, and let users decide which implementation is appropriate for their case.

In any case, you can potentially greatly benefit from Java 8 streams, as that way you can effortlessly parallelize the summing, which can be a huge boost to performance.

\$\endgroup\$
1
\$\begingroup\$

Given the known, limited number of Number types, it might be easier to make separate methods for each type and keep them in one class rather than try to work with generics.

Alternately, clients can use BigDecimal#longValue(), etc.. to get the type they want. Of course, that puts the burden on them if the BigDecimal overflows or underflows and they get an INFINITY result.

You can use sum.add(new BigDecimal(number.doubleValue())) instead of sum.add(new BigDecimal(number.toString())). Not much prettier, but at least you're not doing String conversion.

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