8
\$\begingroup\$

I wrote a method which computes the ratio of even to odd numbers in an array. I know it's a simple piece of code, but I wanted to see if you have any feedback for improving it.

public double percentEven(int[] a) {
if(a.length <= 1) {
    return 0.0;
}
double even = 0.0;
for(int i = 0; i < a.length; i++) {
    if(a[i] % 2 == 0) {
        even++;
    } 
}
return (double) (even / a.length) * 100; 
}

If the array contains zero elements, or zero even elements, return 0.

\$\endgroup\$
1
  • \$\begingroup\$ In the case of an array containing 0 elements I would return something else then 0 because an average over 0 elements is not defined. Either an exception or something like -1. \$\endgroup\$
    – Pieter B
    Jun 19, 2015 at 9:11

3 Answers 3

11
\$\begingroup\$

Potential bug: What if the array contains a single element, and it is even?

if(a.length <= 1) {
    return 0.0;
}

That's what happens.

Easy fix, change to:

if(a.length == 0) {

Your indentation is slightly screwed up, but that might just be a copy-paste error.


double even should be int even.


You could use a for-each loop,

for (int i : a) {
    if(i % 2 == 0) {

int[] a could be named int[] array or int[] values or int[] input.


When using even as an int, use the following return statement to cast only even to a double, which will then make it use double division and not int division:

return (double) even / a.length * 100;

Alternatively you could use:

return even * 100.0 / a.length;

But I think return (double) even / a.length * 100; is better.

\$\endgroup\$
4
  • \$\begingroup\$ Nice! I implemented all of your suggestions. Thanks for all the great feedback! \$\endgroup\$
    – cody.codes
    Jun 19, 2015 at 0:05
  • \$\begingroup\$ @cody.codes Just out of curiousity: What did you decide to name the a variable? \$\endgroup\$ Jun 19, 2015 at 0:07
  • \$\begingroup\$ I usually choose array, but sometimes it confuses me when I don't code for awhile because then I think the object type is "array" when really it's "<insert primitive type>[]." I'll try the other suggestions, like input and value, and see how it goes. Thanks! \$\endgroup\$
    – cody.codes
    Jun 19, 2015 at 0:12
  • 2
    \$\begingroup\$ @cody.codes int[] actually means "integer array". The name I would recommend the most is probably values \$\endgroup\$ Jun 19, 2015 at 0:23
10
\$\begingroup\$

If you're using Java 8, IntStream can be used to solve the problem along with its filter and count methods:

public static double percentEven(int[] values) {
  IntStream evens = IntStream.of(values).filter(x -> x % 2 == 0);
  return (double) evens.count() / values.length * 100;
}

It's not a big win in this particular case, but I think it is a bit cleaner. Processing Data with Java SE 8 Streams, Part 1 has more examples of where streams can be a powerful tool.

Note that unlike the original code, this will return NaN for zero-length arrays. You can of course change it to return 0 instead, if you want.

\$\endgroup\$
1
  • \$\begingroup\$ NaN is better then 0 because an average of 0 elements is not defined. \$\endgroup\$
    – Pieter B
    Jun 19, 2015 at 9:13
4
\$\begingroup\$

I was just surprised by the very short Java 8 solution by mjolka, so I'm giving it a try in Java 7.

public double percentEven(int[] values) {
    int even = 0;
    for (int x : values) even += 1 - (x & 1);
    return 100.0 * even / values.length;
}

I violated the braces-everywhere convention in order to save 2 lines, but this is still not good enough. Java 8 still wins by 1 line, and that seems impossible to beat.

OTOH the Java 8 solution produces some garbage and I'm afraid it's one or two orders of magnitude slower.

Reaction to comments

To those thinking that the following would be clearer

even += (x%2 == 0 ? 1 : 0);

No, it wouldn't. The former is hard to understand for people unfamiliar with bitwise operation, the latter is hard to understand for people unfamiliar with ternary expressions. While bitwise operations are a bit more exotic, it makes no sense to reduces everyone's capabilities to the lowest common denominator. It's no hack, no magic, no code golf, just learn it!

Once we know that x & 1 extracts the lowest binary digit, we could even argue that it's much cleaner than a conditional expression.

Note on optimizations

Division and modulus are pretty expensive and JIT is pretty smart on optimizing them. However, x % 2 is not the same as x & 1 for negative numbers, so more work has to be done, see this answer for some benchmarks.

OTOH x % 2 == 0 is the same as (x & 1) == 0, but I don't know if JIT uses this fact.


A maximally optimized code could look like this

public double percentEven(int[] values) {
    int even = values.length;
    for (int x : values) even -= x & 1;
    return 100.0 * even / values.length;
}

Problems with modulus

Using modulus is a bit error prone as both

even += x%2 == 1 ? 0 : 1;

and

even += 1 - x%2;

are wrong. The problem is that % for negative numbers does not do what we usually need (i.e., rounding towards negative infinity rather than rounding towards zero).

\$\endgroup\$
8
  • \$\begingroup\$ Shouldn't even be cast to a double on the return statement? \$\endgroup\$
    – Scott
    Jun 19, 2015 at 1:55
  • 1
    \$\begingroup\$ @Scott Maybe for clarity, but you'd get a warning ("Unnecessary cast from int to double"). The expression evaluates left to right, so it gets cast anyway because of the multiplication by 100.0 and the division takes place last. I guess, adding parentheses would make sense. \$\endgroup\$
    – maaartinus
    Jun 19, 2015 at 2:10
  • 1
    \$\begingroup\$ Cool. I didn't notice the significance of you swapping the order of even and 100.0 in your answer \$\endgroup\$
    – Scott
    Jun 19, 2015 at 2:11
  • 2
    \$\begingroup\$ even += 1 - (x & 1); is clever, but very unclear and therefore not good code. Something like even += (x%2 == 0 ? 1 : 0) is just as short, but requires much less thought to figure out. \$\endgroup\$ Jun 19, 2015 at 7:56
  • 1
    \$\begingroup\$ @BlueRaja-DannyPflughoeft To someone proficient with bitwise operations it's perfectly clear. There's no hack there, not even a tiny one, just a use of something less well-known. People ignorant of ternary operators could call your code very unclear and require an "if". My answer is: learn the language well. It's not clever, it's no corner case, it's no hack, it's something every HashMap uses. It's also much faster, unless JIT can optimize you code to mine (maybe it can, I don't know). \$\endgroup\$
    – maaartinus
    Jun 19, 2015 at 11:41

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

Not the answer you're looking for? Browse other questions tagged or ask your own question.