0

I have a working algorithm in Python which I want to convert to C++:

def gcd(a, b):    
    if (a % b == 0):
        return b
    else:
        return gcd(b, a % b)

def solution(N, M):
    lcm = N * M / gcd(N, M)
    return lcm / M

I'm having a problem with large input values as the multiple of N and M causes integer overflow and using long to store its value doesn't seem to help, unless I'm doing something wrong.

Here's my current code:

int gcd(int a, int b)
{
    if (a % b == 0)
        return b;
    else
        return gcd(b, a % b);
}

int solution(int N, int M) {

    // Calculate greatest common divisor
    int g = gcd(N, M);

    // Calculate the least common multiple
    long m = N * M;
    int lcm = m / g;

    return lcm / M;
}
Improve this question
5
  • What values are you providing to solution? Commented Feb 22, 2014 at 21:33
  • Both N and M are within range (1, 1 000 000 000). Commented Feb 22, 2014 at 21:34
  • Well, a quick solution would be to use long long. That doesn't mean larger values are all safe, though. Commented Feb 22, 2014 at 21:35
  • But is N*M in range? You could try using long long; on your platform int and long might be the same size. Commented Feb 22, 2014 at 21:36
  • Possible duplicate of Convert Python program to C/C++ code? Commented Nov 21, 2019 at 13:39

3 Answers 3

Reset to default
1

You are computing g=gcd(N,M), then m=N*M, then lcm=m/g, and finally returning lcm/M. That's the same as returning N/gcd(N,M). You don't need those intermediate calculations. Get rid of them. Now there's no problem with overflow (unless M=0, that is, which you aren't protecting against).

int solution(int N, int M) {
   if (M == 0) {
      handle_error();
   }
   else {
      return N / gcd(N,M);
   }
}
Improve this answer
Sign up to request clarification or add additional context in comments.

3 Comments

This is it, well spotted! I hate missing stuff like that. Error handling is not necessary as both M and N are guaranteed to be at least 1.
Though the answer avoids the problem in OP solution, this doesn't highlight the problem in that. Its better to know the actual problem that exists in OP solution. The mistake is very common which is often overlooked.
The intent should be 'why my solution does not work' rather than 'just give me the correct solution'.
1

To begin with, change:

long m = N * M;
int lcm = m / g;

To:

long long m = (long long)N * M;
int lcm = (int)(m / g);

In general, you might as well change every int in your code to unsigned long long...

But if you have some BigInt class at hand, then you might want to use it instead.

Here is one for free: http://planet-source-code.com/vb/scripts/ShowCode.asp?txtCodeId=9735&lngWId=3

It stores a natural number of any conceivable size, and supports all arithmetic operators provided in C++.

Improve this answer

5 Comments

I've tried multiple combinations of this approach and it still doesn't seem to work.
You didn't say why this approach doesn't work, Marian. Are you getting a compile error with long long? That's not part of C++03, but it is a fairly standard extension. It is part of C++11.
No compile time errors, but the result is still incorrect. You can test it for yourself here: codility.com/demo/take-sample-test/chocolates_by_numbers
The problem is that int lcm = (int) (m/g) is incorrect. It needs to be long long lcm = m / g.
Well, as I said in the answer, you might as well change every int in your code to unsigned long long.
-1

The problem is in long m = N*M. The multiplication takes place as 32 bit integers only. Since both are of int type, overflow occurs.

Correction is long long m = (long long)N*M

Improve this answer

Comments

Your Answer

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

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.