Solution of operator exercise

[hageboeck]

Hello @bernhardmgruber, @sponce,

when I did the operator exercise, I arrived at a bit different solution, and I wanted to post it here to figure out if the two approaches have specific advantages / disadvantages. I created a cleaned up diff to illustrate what I did differently below.

Specific points I want to discuss:

• Non-explicit constructor from int. I find that pretty logical, as this allows for collapsing 6 operators into two. Counterarguments?
• const & I agree that it's totally fine to copy four ints if you test for equality. However, shouldn't we be showing the good practice that would get us better results if this wasn't just two ints per instance?
• Normalisation is this class's private business. People don't need to know how and when we do it.
• I think that having the class always in a fully normalised state is a good thing. What do you think?

--- solution/operators.sol.cpp	2022-02-28 16:12:00.000000000 +0100
+++ operators.cpp	2022-02-28 16:09:42.000000000 +0100
@@ -1,89 +1,77 @@
-#include <cassert>
 #include <iomanip>
 #include <iostream>
 #include <numeric>
 
 class Fraction {
- public:
-  explicit Fraction(int i) : m_num(i), m_denom(1) {}
-  Fraction(int num, int denom) : m_num(num), m_denom(denom) {}
+  public:
+  Fraction(int a) : Fraction(a, 1) { }.  // Delegating constructors are awesome
+  Fraction(int a, int b) : m_num{a}, m_denom{b} {
+    Normalise();   // And I like to have the fraction always in a normalised state
+  }
 
   int num() const { return m_num; }
   int denom() const { return m_denom; }
 
That's our internal business:
-  void normalize() {
-    const int gcd = std::gcd(m_num, m_denom);
-    m_num /= gcd;
-    m_denom /= gcd;
-  }
 
// One overload should do:
-  Fraction& operator*=(int i) {
-    m_num *= i;
-    normalize();
-      return *this;
-   }
-  Fraction& operator*=(Fraction f) {
-    m_num *= f.num();
-    m_denom *= f.denom();
-    normalize();
-    return *this;
-   }
+  Fraction & operator*=(const Fraction & other) {
+    m_num *= other.m_num;
+    m_denom *= other.m_denom;
+    Normalise();
+    return *this;
+  }
 

// Also here we can use one to rule them all:
// The lcm might be unnecessary.
// On the minus side, It might be slow.
// On the plus side, it decreases the likelihood of overflows.
-  Fraction& operator+=(int i) {
-    m_num += i * m_denom;
-    return *this;
-  }
-  Fraction& operator+=(Fraction f) {
-    m_num *= f.denom();
-    m_num += f.num() * m_denom;
-    m_denom *= f.denom();
-    normalize();
+  Fraction & operator+=(const Fraction & other) {
+    const auto lcm = std::lcm(m_denom, other.m_denom);
+    m_num = m_num * (lcm / m_denom) + other.m_num * (lcm / other.m_denom);
+    m_denom = lcm;
+    Normalise();
     return *this;
   }

+  private:
+  void Normalise() {
+    const auto gcd = std::gcd(m_num, m_denom);
+    m_num /= gcd;
+    m_denom /= gcd;
   }
 
- private:
-  int m_num, m_denom;
+  int m_num = 1, m_denom = 1;
 };
 
-std::ostream& operator<<(std::ostream& os, Fraction r) {
-  os << r.num() << "/" << r.denom();
+
+std::ostream & operator<<(std::ostream & os, const Fraction & frac) {
+  os << frac.num() << '/' << frac.denom();
   return os;
 }
 
 // Multiplication
-Fraction operator*(Fraction r, int i) { return r *= i; }
-
-Fraction operator*(int i, Fraction r) { return r * i; }
-
-Fraction operator*(Fraction a, Fraction b) { return a *= b; }
+Fraction operator*(Fraction a, const Fraction & b) { return a *= b; }
 
 // Addition
-Fraction operator+(Fraction r, int i) { return r += i; }
-
-Fraction operator+(int i, Fraction r) { return r + i; }
-
-Fraction operator+(Fraction a, Fraction b) { return a += b; }
+Fraction operator+(Fraction a, const Fraction & b) { return a += b; }
 
 // Equality
-bool operator==(Fraction a, Fraction b) {
-  a.normalize();
-  b.normalize();
+bool operator==(const Fraction & a, const Fraction & b) {
   return a.num() == b.num() && a.denom() == b.denom();
 }
 bool operator!=(const Fraction & a, const Fraction & b) { return !(a == b); }
 
 // Relational
-bool operator<(Fraction a, Fraction b) {
-  return a.num() * b.denom() < b.num() * a.denom();
+bool operator<(const Fraction & a, const Fraction & b) {
+  const auto lcm = std::lcm(a.denom(), b.denom());
+  return a.num() * (lcm/a.denom()) < b.num() * (lcm/b.denom());
 }
 
-bool operator>(Fraction a, Fraction b) { return b < a; }
-bool operator<=(Fraction a, Fraction b) { return !(a > b); }
-bool operator>=(Fraction a, Fraction b) { return !(a < b); }
+bool operator>(const Fraction & a, const Fraction & b) { return b < a; }
+bool operator<=(const Fraction & a, const Fraction & b) { return !(b < a); }
+bool operator>=(const Fraction & a, const Fraction & b) { return !(a < b); }
 
 

 void printAndCheck(const std::string & what, const Fraction & result, const Fraction & expected) {

[bernhardmgruber]

Here are a couple of comments:

Non-explicit constructor from int. I find that pretty logical, as this allows for collapsing 6 operators into two. Counterarguments?

I had the same idea but then noticed that operations get more expensive. Like adding an int to a Fraction is 1 mul and 1 add, whereas adding a Fraction to a Fraction involves some more computation including the normalization. But sure, your solution would work as well and it would be fine if students would implement it this way.

const & I agree that it's totally fine to copy four ints if you test for equality. However, shouldn't we be showing the good practice that would get us better results if this wasn't just two ints per instance?

I used to const & alot when I was younger but recently found myself thinking more and more in value semantics and passing values around. But you are right, in general const & should be preferred.

Normalisation is this class's private business. People don't need to know how and when we do it.

Yes. I had it public when I needed it outside the class for operators. It should be private in the final solution.

I think that having the class always in a fully normalised state is a good thing. What do you think?

Fine for me. But I would not require it too soon during the exercise. It's actually only really needed for the comparisons. Everywhere else, it's bonus.

Regarding your diff:

• the delegating constructor could just be a default argument
• I am not too fond of your implementation of Fraction & operator+=(const Fraction & other) since it is probably slower. But then that does not matter for this exercise, it's about implementing the operators correctly. Also whether the operations could overflow or not are not relevant at that stage.
• The operator operator< seems unnecessary complex as I think overflow should not be the point of this exercise.

[bernhardmgruber]

I would also implement the Fraction class slighly different, as I would prefer hidden friends for the non-member operators. Furthermore, constexpr everything.

We could also add operator>> for reading a Fraction, or subtraction and division. But I think the exercise is already good enough to teach something on operators.

[sponce]

I think all comments I've read here are very good, but probably outside the scope of the exercise, which is simply to find out how to implement operators. I agree that the code proposed in the solution should respect best practices, but it should also not be over complicated. So things like explicit, constexpr and friend I would avoid here personally, just to stay focus and not hide the important points.

[chavid]

+1

Also, I was wondering how this behave with negative numbers ? (especially operator<). If this only valid for positive numbers, it is ok, but shoudl be said.

[bernhardmgruber]

My solution totally disregards negative values :) However, they also don't appear anywhere inside main. So the students can make this case work or not. They are also free to choose a signed or unsigned data type to implement Fraction.

[stale[bot]]

This issue or pull request has been automatically marked as stale because it has not had recent activity. Please manually close it, if it is no longer relevant, or ask for help or support to help getting it unstuck. Let me bring this to the attention of @klieret @wdconinc @michmx for now.

[bernhardmgruber]

I remove myself from the assignees since I am happy with the current state of the exercise.