3
\$\begingroup\$

Today I started to make a class called Fraction, where the class will act just like a fraction does in real mathematics. I am doing this just for the challenge. So far I only made the addition part, because I wanted to know if anybody had any suggestions for me.

#include <iostream> #include <string> #include <cmath> //fmax #include <vector> #include <sstream> //stringstream typedef const std::string constr; namespace Util { template <typename T> constr tostring (T t) { std::stringstream s; s << t; return s.str(); } }//namespace Util namespace math { class Fraction; typedef const double cd; typedef const long cl; typedef std::vector<Fraction> vf; class Math //for future use { private: public: }; class Fraction //Fractions cannot be of type 1.2/5.6 { private: //variables public: //variables long numerator; long denominator; private: //functions public: Fraction(); Fraction(cl numerator_, cl denominator_); ~Fraction(); Fraction operator+ (const Fraction num); Fraction operator+ (cl num); //should be possible, for mixed numbers (1 1/2 = 1+1/2=1/2+1) cl getcommondemon (cl num1, cl num2); vf setcommondemon (const Fraction& num1, const Fraction& num2); constr tostring(void); }; Fraction Fraction::operator+ (const Fraction num) { vf out = Fraction::setcommondemon(*this, num); Fraction nn1 = out.at(0); Fraction nn2 = out.at(1); Fraction output; output.numerator = nn1.numerator+nn2.numerator; output.denominator = nn1.denominator; return output; } cl Fraction::getcommondemon(cl num1, cl num2) { if(num1 == num2) return num1; int max = fmax(num1, num2); int min = fmin(num1, num2); if(max % min == 0) //is max a multiple of min? if yes { return max; } else { return max*min; } } Fraction::Fraction() { numerator = 0; denominator = 0; } Fraction::Fraction(cl numerator_, cl denominator_) { numerator = numerator_; denominator = denominator_; } Fraction::~Fraction() { //destructor } vf Fraction::setcommondemon(const Fraction& num1, const Fraction& num2) { vf output; Fraction out1; Fraction out2; if(num1.denominator == num2.denominator) { output.push_back(num1); output.push_back(num2); return output; } bool num1bigger = false; long max = 0; long min = 0; if(num1.denominator > num2.denominator) { num1bigger= true; max = num1.denominator; min = num2.denominator; } else { min = num1.denominator; max = num2.denominator; } if(max % min == 0) //is max a multiple of min? if yes { cl multi = max/min; if(num1bigger == true) { out2.numerator = num2.numerator*multi; out2.denominator = num2.denominator*multi; out1 = num1; } else { out1.numerator = num1.numerator*multi; out1.denominator = num1.denominator*multi; out2 = num2; } output.push_back(out1); output.push_back(out2); return output; } else { out1.numerator = num1.numerator*num2.denominator; out1.denominator = num1.denominator*num2.denominator; out2.numerator = num2.numerator*num1.denominator; out2.denominator = num2.denominator*num1.denominator; output.push_back(out1); output.push_back(out2); return output; } } constr Fraction::tostring() { return Util::tostring(numerator) + "/" + Util::tostring(denominator); } } //namespace math int main(int argc, char* argv[]) { math::Fraction a(1,6); math::Fraction b(1,3); math::Fraction out = a+b; std::cout << out.tostring() << std::endl; return 0; }
\$\endgroup\$

2 Answers 2

Reset to default
3
\$\begingroup\$

Here's a few pointers:

Do not typedef cv-qualified types. It will confuse everybody who reads your code. Seeing cl is really confusing, seeing const long is completely understandable. So get rid of the cd and constr.

If you want to typedef std::vector<Fraction>, you need to provide a reasonable name for that type... vf while concise conveys precisely zero information. So either do not typedef it, which is fine, or call it FractionVec or Fractions.

Fraction should not have a default constructor. Why would 0/0 be the default?

You either need to provide operator== or change the internal representation to keep things to lowerst terms, cause right now Fraction{1, 2} != Fraction{2, 4}.

Functions with multiple words in the name should either look like getCommonDenom or get_common_denom. Running words together having everything be lowercase makes it hard to read. Also, not sure why either of those functions exist, or how it makes sense that a function named setcommondenom returns a vector<Fraction>??? Also this function is almost certainly wrong given its length, look up Euler's algorithm for gcd and use it.

Rather than defining two separate operator+ member functions (one of them is wrong, it should take a const Fraction& btw), you should create an external friend function and make longs implicitly convert to Fraction:

Fraction() = delete; // no Fraction(long numer, long denom = 1); // doesn't have to be const friend Fraction operator+(const Fraction& a, const Fraction& b); // non-member +

This lets you do 1 + Fraction{2,3} too!

Hope that helps.

\$\endgroup\$
1
\$\begingroup\$

Below I propose my cleaning to your code. Some comments:

  • don't use typedef to spare key-typing: it is best to let the code be verbose

  • maybe you can use it for future flexibility (as I did in my sample below). Maybe in the future you want to parameterize your class over integer type

  • if you want to define a general purpose class, the less includes the better (less dependencies).

  • a fraction can be represented in many different ways. It is a good choice to guarantee that it is always in normalized form. This is achieved by a Fraction::normalize private function. To assure that the user does not violate this, data members should be private. If needed you can write public method to access the values

  • if you put a denominator=1 default value in the constructor you have implicit cast from integers to fractions. No need to define two different operator+

  • why to use stringbuf to convert fraction to a string? Provide an operator<< to write to a stream and let the user use stringbuf if he really needs a string.

  • define a GCD function ones for ever. Check it carefully for boundary cases! (I wrote it quickly, I'm not really sure my version is correct). Once you have a well founded GCD function, always refer to it to compute common denominator and to normalize your fraction.

  • don't use const where it is not needed

here is a possible cleanup of your code:

#include <iostream> namespace math { class Fraction //Fractions cannot be of type 1.2/5.6 { private: typedef long value_type; value_type numerator; value_type denominator; public: Fraction(value_type numerator_, value_type denominator_=1) : numerator(numerator_), denominator(denominator_) { normalize(); } Fraction operator+ (const Fraction &other) const { value_type c = gcd(denominator,other.denominator); value_type m_this = denominator / c; value_type m_other = other.denominator / c; return Fraction(numerator * m_other + other.numerator * m_this, denominator * m_other); } static value_type gcd(value_type a, value_type b); friend std::ostream &operator<<(std::ostream &out, const Fraction &fraction) { return out << "(" << fraction.numerator << "/" << fraction.denominator << ")"; } private: void normalize(); }; Fraction::value_type Fraction::gcd(value_type a, value_type b) { if (a<0) a=-a; if (b<0) b=-b; for (;;) { if (a>b) { if (b==0) return a; a -= a/b*b; } else { if (a==0) return b; b -= b/a*a; } } } void Fraction::normalize() { if (denominator<0) { denominator = -denominator; numerator = -numerator; } value_type c = gcd(numerator,denominator); numerator /= c; denominator /= c; } } //namespace math int main(int argc, char* argv[]) { math::Fraction a(1,-6); math::Fraction b(1,3); std::cout << a << " + " << b << " = " << a+b << std::endl; std::cout << a << " + " << 1 << " = " << a+1 << std::endl; return 0; }
\$\endgroup\$

You must log in to answer this question.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.