0

I am trying to sum every other digit in the card number by doing this:

/*
Return the sum of the odd-place digits.
*/

public static int sumOfoddPlace(long number)
{
    int maxDigitLength = 16;
    int sum = 0;
    for (int i = 1; i <= maxDigitLength; i++)
    {
        if (i % 2 == 1)
        {
            sum = sum + (int)(number % 10);
        }
        break;
    }
    return sum;
}

All I get is 6. The sum I am looking for is supposed to be 37.

PaulG
  • 13,871
  • 9
  • 56
  • 78
Dan Gurewitch
  • 5
  • 2
  • 2
  • 4

4 Answers4

1

You're breaking out of the loop on the very first iteration only. So, you won't go past to another iteration.

However, removing the break too won't solve your problem. number % 10 will always give you the last digit of the number, and not every alternate number. You should follow this approach:

  • num % 10 - Will give you last digit.
  • Then update the num by trimming off the last 2 digits.
  • Repeat
Rohit Jain
  • 209,639
  • 45
  • 409
  • 525
1

Try this ... this should work for you

     public static int sumOfoddPlace(long number)
     {
         int maxDigitLength = 16;
         int sum = 0;
         for (int i = 0; i < maxDigitLength; i++)
         {
             if (i % 2 != 0)
             {
                 sum =  (sum + (int)(number % 10));
                 number =  number/10;

             }else {
                 number =  number/10;
            }

         }
         return sum;
 }

What I have done here is if i is odd, I take a mod of the number so I get the last digit of the number and then add it to sum and then I get rid of the last digit by dividing it with 10 and if the number is even I just get rid of the digit in the ith position.

Here I am collecting the digits in odd places in reverse order.

BlackVegetable
  • 12,594
  • 8
  • 50
  • 82
  • 1
    You can eliminate the `else` by moving `number = number / 10;` outside (and after) the `if` statement. Actually, you should be able to cut the number of iterations in half.... What other improvements can you find (remember casting is usually an expensive operation) – Clockwork-Muse Oct 30 '13 at 11:36
  • Actually, now that I look at this a little harder, you're collecting the _other_ digits - you need to flip your condition around (given you changed the initial value of the `for` loop). – Clockwork-Muse Oct 31 '13 at 08:53
0

I haven't seen the minimum solution yet, so here goes:

public static int sumOddDigits(long input) {
    int sum = 0;

    for (long temp = input; temp > 0; temp /= 100) {
        sum += temp % 10;
    } 
    return sum;
}

You don't need to divide by 10 and check if it's an even number, you can just divide by 100 every time.

Demo: http://ideone.com/sDZfpU

Hans Z
  • 4,664
  • 2
  • 27
  • 50
  • Initializing `sum` in the for-loop seems slightly strange - I'd just leave the initialization section of the loop blank (and initialize `sum` with it's declaration). – Clockwork-Muse Oct 30 '13 at 22:34
  • @Clockwork-Muse I did that so people don't freak out at empty initialization statement in my for loop. I made it nicer now. – Hans Z Oct 30 '13 at 22:42
-1

Here is updated code in which i have removed logic of flag.This is shorter and easier to understand.

public static int sumOfOddDigits(long number){
    int sum = 0;
    String newString = new StringBuilder(String.valueOf(number)).reverse().toString();
    number = Long.parseLong(newString);

    while (number != 0){
        sum = (int) (sum + number % 10);
        number = number / 100;
    }
    return sum;
}
Rushabh Shah
  • 178
  • 1
  • 4
  • 13
  • -1 Seriously, what's up with this version? It looks like it _should_ work... except that it's overly complicated and violates a number of the Java coding conventions. – Clockwork-Muse Oct 30 '13 at 11:41
  • @Clockwork-Muse I am new to programming that's why it may be possible that it violates the Java Coding standards but as far as code is concern, it is working completely and it really doesn't seems to complicated... – Rushabh Shah Oct 30 '13 at 12:40
  • Okay then... you're using a `flag` value, when you should be using a boolean. You copy the input number into a string, transfer it into a different string (in a completely inefficient and bizarre manner, which **reverses** the string), then _back_ into a number. In order for you to get this far (and follow some of the conventions you do), you should have encountered several features that should be showing up here. At best, this method is dangerous and wrong (it's ignoring trailing zeros which gives even/odd digits!)... – Clockwork-Muse Oct 30 '13 at 13:03
  • okay i got it, i will keep mind these things and soon will come up with all changes you have suggested...Thank You @Clockwork-Muse – Rushabh Shah Oct 30 '13 at 13:10
  • ...The code still has one major problem - it's still ignoring trailing zeros (so still dangerous/wrong) - what's the point of reversing it? Or even converting it to a string? – Clockwork-Muse Oct 30 '13 at 22:32
  • Here i am deviding number with 100 so it will give top digits with two less digits every time. So if the number is even then it will give odd numbers but when the number is odd it will give even numbers which is not required So that's why i reverse the string... Ex. if number is 123 then i get 3 and 1 every time and if number is 1234, i don't reverse string i will get 4 and 2 digits every time which is not as per our logic.... Now if i reverse the string and if number is 4321 then i get 3 and 1 and if number is 321 then i get 3 and 1 which is as per our logic... So i am reversing string. – Rushabh Shah Oct 31 '13 at 07:29
  • I see what you're going for there, but unfortunately, check digits are usually counted from the **right**. Which is how numbers are actually indexed too (1s, 10s, etc). Some algorithms even depend on knowing the actual index! You can't just 'remove' information the way you're doing (especially as the effect is somewhat hidden). – Clockwork-Muse Oct 31 '13 at 08:51