11
\$\begingroup\$

I have code that converts each character of a String to an int and returns the difference between odd and even numbers. Can this code be simplified?

int compareSumOfDigits(String N) {
int e=0,o=0;
for (int i =0;i<N.length();i++){
    int t = Character.getNumericValue(N.charAt(i));
    if(t%2==0)
    e+=t;
else
    o+=t;

}
      return  o-e ;
}
\$\endgroup\$
  • \$\begingroup\$ @DavidMatriguet That's heavily depend on the definition of "more simple code". Except for the inexplicable variable names (it's hard to get, what e, o and N from a first glance), the code is pretty straightforward and clear. \$\endgroup\$ – m0nhawk Aug 25 '15 at 11:08
  • \$\begingroup\$ @m0nhawk, formatting a question's indentation and spacing removes reviewers of a point to focus on, and damages the intent of the initial question. \$\endgroup\$ – Quill Aug 25 '15 at 11:12
  • \$\begingroup\$ @m0nhawk As Quill says above, please note that on Code Review, we prefer to post an answer about the formatting rather than fixing it for the OP. \$\endgroup\$ – Simon Forsberg Aug 25 '15 at 11:13
19
\$\begingroup\$

Combining some of the existing answers, you can do

s.chars()                          // get the char stream
  .map(Character::getNumericValue) // convert to ints
  .map(n -> n%2==0 ? -n : n)       // negate the even ones
  .sum()                           // sum it all up

This will give you the sum of the odds and the negative evens, which is the same as the sum of the odds minus the sum of the evens.

edit

In response to @kai, for absolute max readability, I'd probably do (pseudocode)

List<int> ints = s.chars().map(Character::getNumericValue)
Map<Boolean, List<int>> intsEven = ints.partitioningBy(n -> n%2==0)
return intsEven.get(false).sum() - intsEven.get(true).sum()

or with isEven from the other answers and not defined here

List<int> ints = s.chars().map(Character::getNumericValue)
List<int> evens = ints.filter(isEven)
List<int> odds = ints.filter(not(isEven))
return odds.sum() - evens.sum()
\$\endgroup\$
  • \$\begingroup\$ A clever solution, I think it might be a little too clever even (if we're "optimizing for readability"). I might have gone for a more verbose approach (that doesn't require comments or reading twice or even thrice), but still an upvote from me! \$\endgroup\$ – sara Aug 26 '15 at 16:05
  • \$\begingroup\$ @kai added slightly more verbose one above. \$\endgroup\$ – Dax Fohl Aug 26 '15 at 16:40
  • 1
    \$\begingroup\$ Even though I have never used Java8, I would definitely prefer solution 1 with the comments. Comments make it easy to understand what is going on, and then it's trivial to see how the code is correct. The more verbose solutions require a lot more thinking to be certain they are correct (at least for me). \$\endgroup\$ – hyde Aug 26 '15 at 16:55
  • \$\begingroup\$ 18 votes in and no comment that this has suggested negating the odd numbers when the question indicated likewise? Or am I missing something here? \$\endgroup\$ – h.j.k. Aug 27 '15 at 3:02
  • 1
    \$\begingroup\$ @h.j.k. fixed in most recent edit. \$\endgroup\$ – Dax Fohl Aug 27 '15 at 13:17
14
\$\begingroup\$

Instead of counting two sums (of odd and even digits) and returning their difference, you could use a single sum value, subtracting a digit's value if it's even and adding if it's odd.

An added benefit of this approach is that you are more protected from integer overflows: if odd and even digits are interleaved, the two-sum approach will be much less likely to overflow.

Building on @m0nhawk's version, with further improving the variable names and a few other tidbits:

boolean isEven(int number) {
    return (number % 2) == 0;
}

int compareSumOfDigits(String numericString) {
    int sum = 0;
    for (Character ch : numericString.toCharArray()) {
        int digit = Character.getNumericValue(ch);
        if (isEven(digit)) {
            sum -= digit;
        } else {
            sum += digit;
        }
    }
    return sum;
}

Or as @tobias_k proposed, the if-else can be flattened for a more compact form, but this is less readable so it goes away from "simple", and I don't recommend it, but here you go anyway:

int compareSumOfDigits(String numericString) {
    int sum = 0;
    for (Character ch : numericString.toCharArray()) {
        int digit = Character.getNumericValue(ch);
        sum += digit * (isEven(digit) ? -1 : 1);
    }
    return sum;
}

Nothing to do with simplicity, but when playing with an implementation, it helps to have some JUnit tests handy to verify that the code still works. A few examples to get you started:

@Test
public void test_compareSumOfDigits_11111111() {
    assertEquals(8, compareSumOfDigits("11111111"));
}

@Test
public void test_compareSumOfDigits_22222222() {
    assertEquals(-16, compareSumOfDigits("22222222"));
}

@Test
public void test_compareSumOfDigits_12345678() {
    assertEquals(-4, compareSumOfDigits("12345678"));
}
\$\endgroup\$
  • \$\begingroup\$ digit * (isEven(digit) ? -1 : 1); or isEven(digit) ? -digit : digit; \$\endgroup\$ – njzk2 Aug 25 '15 at 19:54
  • \$\begingroup\$ @njzk2 to make it even more compact, you can kill the helper function and just use the mod's value: digit * (((number%2)*2)-1). \$\endgroup\$ – Dax Fohl Aug 25 '15 at 20:09
  • \$\begingroup\$ You could replace (number % 2) == 0 with !(number & 1) or (number & 1) == 0. But that may end up harder to read, but it surelly is faster! I'm not sure how Java would work with that. \$\endgroup\$ – Ismael Miguel Aug 26 '15 at 10:06
  • \$\begingroup\$ Is an overflow really a problem here? As long as the overflow is followed by an underflow your end result should be correct regardless of the order of calculation. \$\endgroup\$ – Dorus Aug 26 '15 at 13:29
7
\$\begingroup\$

It depend on you definition of a "more simple code". From my point of view it can be extend with the next stuff:

  • Formatting, obviously.
  • Using foreach loop, instead of indexing (notice, that you use index once in the for, only to get a character), this will greatly simplify the code.
  • Introduce function isEven for check. Java compiler is smart enough to reduce to zero all overhead on calling this function, but from reader view it's clearer now.
  • Put more expressible variable names, instead of one letter. I can hardly recall any situation where it impossible to seek for a better, explanatory name.

So, my simplier code looks like this.

Boolean isEven(int number) {
    return (number % 2) == 0;
}
int compareSumOfDigits(String numbers) {
    int sumEvens = 0, sumOdds = 0;
    for (Character ch : numbers.toCharArray()) {
        int number = Character.getNumericValue(ch);
        if (isEven(number)) {
            sumEvens += number;
        } else {
            sumOdds += number;
        }
    }
    return sumOdds - sumEvens;
}
\$\endgroup\$
  • 2
    \$\begingroup\$ "ntroduce function isEven for check. Java compiler is smart enough to reduce to zero all overhead on calling this function, but from reader view it's clearer now." -- not really. I can read number % 2 == 0 just fine, and I don't have to pull up the documentation to use it. \$\endgroup\$ – John Dvorak Aug 25 '15 at 14:48
  • 1
    \$\begingroup\$ I agree with @JanDvorak here... otherwise sum(a, b) and difference(a, b) are just begging to be implemented. \$\endgroup\$ – Jacob Raihle Aug 25 '15 at 15:51
  • \$\begingroup\$ isEven(int) is an obvious candidate for refactoring to a helper method for me. n % 2 == 0 is easy and very common, true, but it is implementation noise and not at all related to the algorithm. I don't see how it is equal to implementing sum(int, int) and so on. Checking for evenness contains logic. It does an operation and examines the result. Addition and subtraction is just an arithmetic operation. \$\endgroup\$ – sara Aug 26 '15 at 16:11
4
\$\begingroup\$

If by "simplified" you mean "shorter", you could create a two-element array to store the sums, where sum[0] is even and sum[1] is odd. Then you can in one line do sum[digit%2] += digit rather than the five-line if/else block.

\$\endgroup\$
  • \$\begingroup\$ Hmm... this is a quite smart idea \$\endgroup\$ – Simon Forsberg Aug 25 '15 at 14:43
  • \$\begingroup\$ @SimonAndréForsberg clever, maybe; succinct, sure; smart, probably not. I'd not do this in my own code. \$\endgroup\$ – Dax Fohl Aug 26 '15 at 15:32
4
\$\begingroup\$

This is pretty subjective. You could use Java 8's new lambda functions, if you find reading them to be simple/elegant. However, this iterates through your String twice, and any more sophisticated lambdas might give you more code bloat.

Let me know if anyone can find a more elegant solution with lambdas!

int compareSumOfDigits( String s )
{
    int sumOdds = s.chars().map( Character::getNumericValue )
       .filter( n -> n % 2 != 0 ).sum();

    int sumEvens = s.chars().map( Character::getNumericValue )
       .filter( n -> n % 2 == 0 ).sum();

    return sumOdds - sumEvens;
}
\$\endgroup\$
  • 1
    \$\begingroup\$ I would do .map(n -> n % 2 == 0 ? -n : n) instead of the 2 filters and that's it. (only one iteration, then) \$\endgroup\$ – njzk2 Aug 25 '15 at 19:53
  • \$\begingroup\$ @njzk2 Yeah, that's totally fair, I think it takes longer to understand what's happening - it makes the code a bit more confusing - but is a good solution considering you save a second collection traversal. \$\endgroup\$ – ultimate_guy Aug 25 '15 at 20:22
1
\$\begingroup\$

If performance is any concern, then you can increase your speed by using bitwise operations.

To check if the integer is odd, all you have to do is check the 1st bit of the integer: (t & 1).

To transform t into the range +/- t you can use the form y = mx + b where:

  • b = minimum value = -t
  • m = distance from b to maximum value = 2t
  • x = is odd = (t & 1) (therefore having only the values 0 or 1)

This eliminates the need for a conditional statement, and the use of modulo. Your function could now be written as:

static int compareSumOfDigits(String N) {
    int sum = 0;
    for (int i =0;i<N.length();i++){
        int t = Character.getNumericValue(N.charAt(i));
        sum += (2*(t & 1) - 1)*t;
    }
    return sum;
}

Comparing the speed between this function and the original function over 1000000 trials, I found the above function using bitwise operations to run twice as fast.

\$\endgroup\$
0
\$\begingroup\$

You can count just the even digits, or just the odd digits. If you count the odd ones, then clearly e = N.length - o, and instead of returning o - e you return 2 * o - N.length.

And instead of an if statement, you count the odd digits by summing the values of t % 2.

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