HiveBrain v1.2.0
Get Started
← Back to all entries
patterncppMinor

Fraction program with Fraction class

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
withclassprogramfraction

Problem

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
#include
#include //fmax
#include
#include //stringstream

typedef const std::string constr;

namespace Util
{
template
constr tostring (T t)
{
std::stringstream s;
s 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_)
{
nume

Solution

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, 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??? 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.

Code Snippets

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 +

Context

StackExchange Code Review Q#33358, answer score: 3

Revisions (0)

No revisions yet.