Project Euler #8, I can't figure out why my code gives absurdly high values

Question

Recently, I started with Project Euler and so far it's been going well, but I went on to the 8th problem, and have been stuck on it ever since.

In the problem there is a 1000-digit number and we need to find the greatest product of the adjacent 13 numbers. The number is here:

73167176531330624919225119674426574742355349194934 96983520312774506326239578318016984801869478851843 85861560789112949495459501737958331952853208805511 12540698747158523863050715693290963295227443043557 66896648950445244523161731856403098711121722383113 62229893423380308135336276614282806444486645238749 30358907296290491560440772390713810515859307960866 70172427121883998797908792274921901699720888093776 65727333001053367881220235421809751254540594752243 52584907711670556013604839586446706324415722155397 53697817977846174064955149290862569321978468622482 83972241375657056057490261407972968652414535100474 82166370484403199890008895243450658541227588666881 16427171479924442928230863465674813919123162824586 17866458359124566529476545682848912883142607690042 24219022671055626321111109370544217506941658960408 07198403850962455444362981230987879927244284909188 84580156166097919133875499200524063689912560717606 05886116467109405077541002256983155200055935729725 71636269561882670428252483600823257530420752963450

My code goes as follows:

int main() {
char str[] = "7316717653133062491922511967442657474235534919493496983520312774506326239578318016984801869478851843858615607891129494954595017379583319528532088055111254069874715852386305071569329096329522744304355766896648950445244523161731856403098711121722383113622298934233803081353362766142828064444866452387493035890729629049156044077239071381051585930796086670172427121883998797908792274921901699720888093776657273330010533678812202354218097512545405947522435258490771167055601360483958644670632441572215539753697817977846174064955149290862569321978468622482839722413756570560574902614079729686524145351004748216637048440319989000889524345065854122758866688116427171479924442928230863465674813919123162824586178664583591245665294765456828489128831426076900422421902267105562632111110937054421750694165896040807198403850962455444362981230987879927244284909188845801561660979191338754992005240636899125607176060588611646710940507754100225698315520005593572972571636269561882670428252483600823257530420752963450";
unsigned long long int  product,maxPrev=0;
int i = 0, j = 0;
product = (str[i] - 48) *(str[i+1] -48) *(str[i+2] -48) *(str[i+3] -48) *(str[i+4] -48) *(str[i+5]-48) *(str[i+6]-48) *(str[i+7] -48) *(str[i+8] -48) *(str[i+9]-48) *(str[i+10]-48) *(str[i+11]-48) *(str[i+12]-48);
maxPrev = product;
for(i=0;i<=986;i++){
       product= (str[i] - 48) *(str[i+1] -48) *(str[i+2] -48) *(str[i+3] -48) *(str[i+4] -48) *(str[i+5]-48) *(str[i+6]-48) *(str[i+7] -48) *(str[i+8] -48) *(str[i+9]-48) *(str[i+10]-48) *(str[i+11]-48) *(str[i+12]-48);
       if(product>maxPrev){
               maxPrev = product;
                        }                     
}
printf("\n%llu",maxPrev);
getchar();
return 0;

}

So far, I noticed that somewhere in the code, an overflow occurs and I can't figure out why.

Answer 1

OP code is certainly having overflow due to int multiplication rather than using a wider type like long long or unsigned long long @mch.

Yet I see the problem may be more efficiently solved by forming the first product and then dividing out the eldest digits and the multiplying by the new digit. Special handling of 0 digits allows the digit scanning to skip ahead 13 digits and need not cause a divide-by-0.

I now see this is part of @Martin Bonner comments. So will make it wiki.

Sample code: cpp.sh/9vuif

Answer 2

EDIT2: Explanation of the problem

Your issue was that when you computed product = A * B * C * D * ... your intermediate results (A, B, ...) were of type int by default. The C standard states that multiplying two int's produces another int, it won't automatically upgrade itself to a bigger type if the value is too big. As such, the code was the same as:

int int_product = A * B * C * D * ...;
product = (unsigned long long int) int_product;

Which shows clearly why there would be such an error. To prevent the compiler from treating the intermediate results as ints you have 2 options:

1. Never store the intermediate results in the first place.

This is the option I present below, where you never actually multiply two integers together, rather you always multiply the unsigned long long int with an int. The reason this works is that the C standard states that the int will be promoted to match the unsigned long long int. In general when there are two types of different sizes, the compiler will tend to promote the smaller one to match the larger one if necessary rather than the other way around. Based on this we can construct:

2. Convert one of the intermediate results to the appropriate sign to force integer promotion.

This is the option presented in the other answer. By changing the statement to:

product = (unsigned long long int) A * B * C * D * ...;

You force the coercion of A * B to be unsigned long long int which forces (A * B) * C to be the same, etc all the way until the end of the values.

Unfortunately very soon you will find PE questions where the numbers really are too big for an unsigned long long int to store. In this case you will need bignums. When I did my PE spree I used the gnu multiprecision library, gmp. It is fast and relatively easy to use.

---

The largest value 13 adjacent digits multiplied together can have is 9^13 which is about 2 500 000 000 000. That may very well be larger than your unsigned long long int can hold. You should check this manually by just computing 9^13 and seeing if your value can hold it.

If that is not the problem, make sure that -48 is correct. For that reason I would switch to -'0'. I would also check to make sure that the number has no newlines/spaces in it. If your string contains newlines or spaces then subtracting '0' from it will indeed (correctly) produce negative values. If you copy-paste that block directly into the code, you will be dropping newlines (ascii code 10) and getting negatives.

tl;dr: You probably have non-digits in your string, remove them and it should work.

EDIT: Your problem is integer promotion. You need to ensure that at every step you are using an unsigned long long int. As such, use that 'j' variable you created as follows:

for (i = 0; i <= 986; i++) {
    product = (str[i] - '0');
    for (j = 1; j <= 13; j++) {
         product *= str[i + j] - '0';
    }
    // ...
}

That way you never try to multiply things in anything other than an unsigned long long int.

Modifying your code that way I was able to produce the result expected from PE.

Answer 3

It is because (str[i] - 48) evaluates to an int, not an unsigned long long.

You have to cast the expression

product = (unsigned long long)(str[i] - '0') *(str[i+1] - '0') *(str[i+2] - '0') *(str[i+3] - '0') *(str[i+4] - '0') *(str[i+5] - '0') *(str[i+6] - '0') *(str[i+7] - '0') *(str[i+8] - '0') *(str[i+9] - '0') *(str[i+10] - '0') *(str[i+11] - '0') *(str[i+12] - '0');

Also you should use '0' instead of 48.

Instead of this line you can use another loop

product = 1;
for (int j = 0; j < 13; j++)
{
    if (!isdigit(str[i + j]) || str[i + j] == '0')
    {
        product = 0;
        break;
    }
    product *= str[i + j] - '0';
}

to calculate the product. In this case the cast is not necessary, because product has already the correct type.