3
\$\begingroup\$

I have found the answer to the problem I am trying to solve, but I am curious if there can be any improvements made to my code.

#include <iostream>
#include <limits>
#include <cmath>
using namespace std;

unsigned long pythagoreanTriplet();
int main() {
cout << pythagoreanTriplet() << endl;
}

unsigned long pythagoreanTriplet()
{
int a,b;
   unsigned long c;
for(a = 1; a < 1000;a++)
{
    for(b = a+1; b <= 1000;b++)
    {
        c = sqrt(pow(a,2)+pow(b,2));

        if(pow(a,2)+pow(b,2) == pow(c,2))
        {
            if(a+b+c == 1000)
            {
                unsigned long product = a*b*c;
                return product;
            }
        }
    }
 }
}
\$\endgroup\$
5
\$\begingroup\$

Don't use std::pow(n, 2) to square an integer - not only is it slower than n*n, it's much less accurate (so you'd want to add 0.5 before assigning to c, to round to nearest).


Instead of computing c as the hypotenuse of triangle (a,b,c), it's simpler to take c as 1000-a-b, then check if that makes a right-triangle:

        c = 1000 - a - b;
        if (a*a + b*b == c*c) {
            return a*b*c;
        }

Look, no <cmath>!


We can reduce the limits of our search, by noting that a can't be as much as 500 (because a < b and a+b < 1000):

for (a = 1;  a < 500;  ++a) {
    for (b = a+1;  b < 1000-a;  ++b) {
        c = 1000 - a - b;

Keep your types consistent. You seem to be aware that int is only guaranteed to be able to hold values up to 32,767, so that's obviously the wrong choice for c and for the result. But it's also the wrong choice for a and b, because expressions such as a*b can overflow (500*500 = 250,000), so that computing the return value could be Undefined Behaviour. The best approach is to use one of the fixed-size types defined in <cstdint> for all three quantities - I recommend std::uint_fast32_t for this.


Make sure you return a value even if no match was found. At present, we just run off the end of the function (and get a compiler warning, so we have no excuse for missing that). We can safely return zero to indicate "no match", as that could never be the result of a successful search.


Modified code

#include <iostream>
#include <cstdint>

std::uint_fast32_t pythagoreanTriplet()
{
    for (std::uint_fast32_t a = 1;  a < 500;  ++a) {
        for (auto b = a+1;  b < 1000-a;  ++b) {
            auto c = 1000 - a - b;
            if (a*a + b*b == c*c) {
                return a*b*c;
            }
        }
    }
    return 0;                   // not found
}

int main()
{
    std::cout << pythagoreanTriplet() << std::endl;
}

Exercise

Instead of hard-coding the target sum 1000, pass it as a parameter to your function.

\$\endgroup\$
  • 1
    \$\begingroup\$ A bit offtopic but is your use/lack of spaces between operators intended to denote precedence or is this just an artifact of refactoring? \$\endgroup\$ – yuri Jun 2 '18 at 9:25
  • 1
    \$\begingroup\$ It's a bit of both - it's what I find most comfortable to read. I also like to separate the expressions in a for control with double-spaces to make it easier to read. Some of it is retained from the original code (e.g. a+1), but I've obviously changed brace style to my own preference, but I don't object to other styles - as long as they are consistent! \$\endgroup\$ – Toby Speight Jun 2 '18 at 9:30
  • \$\begingroup\$ The pow really does make the code slow down by a lot. Time went from 0.020887 to 0.000489. \$\endgroup\$ – austingae Jun 2 '18 at 17:06
2
\$\begingroup\$

I am not a C++ programmer, so I can't give C++ advice.

  1. Using Euclid's formulas one can solve the problem by using pencil and paper.
  2. Your format is inconsistent and ugly.
  3. Why do you define int a,b but unsigned long c. c has almost the same size as a. So why long? And why unsigned but a and b signed?
  4. I would prefer if you make the conversion of sqrt(pow(a,2)+pow(b,2)) to an integer type explicit.
  5. You stop after you have found the first triple. Why not display all solutions? Actually there is only one solution.
  6. If there is no solution your program returns 0. That is wrong.
  7. It is sufficient to declare c in the for loop.
  8. Don't use magic numbers like 1000. Use constants (or macros in C)
  9. If you use floating point operations than you should be aware of rounding errors. Are you sure that i==(int)sqrt(i*i) is guaranteed? If i<sqrt(i*i) then your algorithm won't work. Actually it is defined in IEEE Floating point standard that this sqrt should be the exact value, and if c++ has implemented this standard (I don't) know, everything is fine. But if you can avoid floating point arithmetic then you should probably do so. You can avoid floating point calculations if you calculate c by c=1000-a-b.
\$\endgroup\$
1
\$\begingroup\$

Certainly! Read through and bookmark the C++ Standard Guidelines. Numbers I note later are citations from this.

Don’t write using namespace std;.

Prefer prefix over postfix

   c = sqrt(pow(a,2)+pow(b,2));

There is a standard library function that does this expression, faster and with more accuracy.

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