C++ calling single helper function with *this attributes

Question

Edit: I’m a beginner to C++, and I’d like to understand more about how to optimize my code.

I have created a Fraction object in C++ as well as overloaded +, - operations etc. When I came to the unary operators, however, I realized I didn't know how to reduce the fraction in the most efficient manner. So I have a function gcd that finds the greatest divisor:

int gcd (int n, int m) {

int newN = n < 0 ? -n : n;
int newM = m < 0 ? -m : m;
if (newM <= newN && newN % newM == 0) { return newM; }
else if (newN < newM) { return gcd(newM, newN); }
else { return gcd(newM, newN%newM); }
}

and then I have an overloaded operator, for example, incrementation:

Fraction& Fraction::operator++() {
num = num + denom;

//reduce fraction
int divisor = gcd(denom,num);
num = num/divisor;
denom = denom/divisor;
if (num < 0 && denom < 0) {num *= (-1);}
if (denom < 0) {denom *= (-1);}

return *this;
}

For efficiency, I would like to put the reduce fraction part of the code in a separate single helper function so the final function would look like this:

Fraction& Fraction::operator++() {
num = num + denom;

//reduce fraction
reduce(num, denom);

return *this;
}

That way I don't have to copy and paste whatever is in //reduce fraction everytime I overload a unary operator for example. However, I'm not sure how the reduce(Fraction num, Fraction& denom) function should look like. At most I can implement it like this:

void reduce(int& num, int& denom) {
int divisor = gcd(denom,num);
num = num/divisor;
denom = denom/divisor;
if (num < 0 && denom < 0) {num *= (-1);}
if (denom < 0) {denom *= (-1);}
}

I'm sure the code above will run into issues during compilation, so I was wondering if I could be suggested any pointers as to efficiently create the reduce fraction function. This is maybe being a bit nitpicky since my original code runs fine, but since I am new to C++, I'd like to learn more about how I can make my code more efficient. Thanks a lot! Let me know if more information is needed.

Edit: The above code does not work. Compiles correctly, but does not reduce fraction properly. So 1/2 + 1/4 results in 6/8, not 3/4.

Answer 1

Well on a high level your gcd function is too complicated and the last part of reduce is a bit wrong. If only denom is negative you invert it. Nicely shows why it's always a good idea to put code into proper functions because they can also be separately tested. So I'd suggest writing some unit tests for your reduce and gcd functions. Start with a simple solution like

static int gcd(int a, int b)
{
    return b == 0 ? a : gcd(b, a % b);
}

and adapt if needed for negative numbers considering % semantics. Thinking about it the function should already be fine like that and you just need to call std::abs(gcd(n,d)) in reduce.

In general you should ask yourself if you really want to pay the renormalization cost at every single operation or if you let the user decide when to call reduce.

For lower-level optimizations here are some hints:

• Always test/measure, e.g. by looking at what the compiler actually produces with godbolt.org.
• The recursion in gcd is not a problem from a performance point of view in this case as it's tail recursive and the compiler will turn it into a loop for you.
• The out parameters in reduce are bad for optimizations cause the compiler has to prove they don't point to the same object. Returning a std::pair and using C++11 std::tie or C++17 structured bindings at the callsite if possible is way more elegant.