patterncppModerate
Implementation of a Rational Number class in C++
Viewed 0 times
implementationnumberclassrational
Problem
First of all, disclaimer; this is for a school assignment. I hope I'm not doing anything inappropriate by asking for input on it, but if it makes a difference, the assignment has already been submitted and graded.
I've been having some problems with the professor that submitted the assignment, but I value my education and want to understand this as much as possible. He just announced in class that he will no longer be answering questions via email, and that we'll have to attend his office hours if we have any. This is a bit of a problem for me as I'm working full time while in school.
The assignment was to implement a Rational number class from a header that he provided. We aren't allowed to modify the contents of the header and are required to implement the class exactly as it's found there.
To make a long story short, there's a section on the submission website where he posts feedback for us on our assignments. My feedback was "Too many errors to list" and nothing else. I'm at a loss, I'm sure my code isn't perfect but I felt like I tested it pretty thoroughly at it always produced the expected output.
If anyone more experienced/knowledgeable than me would be kind enough to review my code and help me understand some of the errors he may be referring to, I'd really appreciate it. I plan on finding a chance to go into his office hours, but he's got a very eclectic way of explaining things and seems to view explaining concepts to students as an inconvenience, so I don't want to put all my eggs in that basket. Its frustrating because I do a ton of outside reading, I'm really motivated to learn and spend a ton of time helping other students in that class understand the concepts that he doesn't lecture on. I'm just at a loss for what I'm missing here.
Here's the code:
As I mentioned, Rational.h is provided by him and we can't change it. I don't like some things about it (like the using namespace std in the header), but I can't do anything about it unfortunately.
I've been having some problems with the professor that submitted the assignment, but I value my education and want to understand this as much as possible. He just announced in class that he will no longer be answering questions via email, and that we'll have to attend his office hours if we have any. This is a bit of a problem for me as I'm working full time while in school.
The assignment was to implement a Rational number class from a header that he provided. We aren't allowed to modify the contents of the header and are required to implement the class exactly as it's found there.
To make a long story short, there's a section on the submission website where he posts feedback for us on our assignments. My feedback was "Too many errors to list" and nothing else. I'm at a loss, I'm sure my code isn't perfect but I felt like I tested it pretty thoroughly at it always produced the expected output.
If anyone more experienced/knowledgeable than me would be kind enough to review my code and help me understand some of the errors he may be referring to, I'd really appreciate it. I plan on finding a chance to go into his office hours, but he's got a very eclectic way of explaining things and seems to view explaining concepts to students as an inconvenience, so I don't want to put all my eggs in that basket. Its frustrating because I do a ton of outside reading, I'm really motivated to learn and spend a ton of time helping other students in that class understand the concepts that he doesn't lecture on. I'm just at a loss for what I'm missing here.
Here's the code:
As I mentioned, Rational.h is provided by him and we can't change it. I don't like some things about it (like the using namespace std in the header), but I can't do anything about it unfortunately.
Solution
In the comments below, where a family of operators all follow a common pattern, I've just addressed one of them; understand that to mean that my comments likely apply equally to the others of that family.
Prefer initialization of members to assignment
Instead of
we normally write
And in modern C++, we delegate to a different constructor:
In truth, if we controlled the interface, we'd just declare the first argument with a default of
The other constructors should also initialize rather than assign:
Alternatively, delegate the copy constructor, too:
Prefer standard exception types
Here you throw a
Instead, we can use the Standard Library exception type for this:
You'll need to include `
int verify(Rational actual, Rational expected, const char *expression)
{
if
Prefer initialization of members to assignment
Instead of
Rational::Rational()
{
_p = 0;
_q = 1;
}we normally write
Rational::Rational()
: _p{0},
_q{1}
{
}And in modern C++, we delegate to a different constructor:
Rational::Rational()
: Rational{0}
{
}In truth, if we controlled the interface, we'd just declare the first argument with a default of
0 and not even need to write a default constructor:Rational(long long = 0, long long = 1);The other constructors should also initialize rather than assign:
Rational::Rational(long long p, long long Q)
: _p{p},
_q{Q}
{
validate(_p, _q);
}
Rational::Rational(const Rational& rat)
: _p{rat._p},
_q{rat._q}
{
}Alternatively, delegate the copy constructor, too:
Rational::Rational(const Rational& rat)
: Rational{rat._p, rat._q}
{
}Prefer standard exception types
Here you throw a
const char*:if (rat._p == 0)
throw "Division by zero not allowed";Instead, we can use the Standard Library exception type for this:
if (rat._p == 0)
throw std::domain_error{"Division by zero not allowed"};You'll need to include `
for this.
Inline the argument validation
Instead of a stand-alone validate() function, it's clearer to test the argument in the constructor:
Rational::Rational(long long p, long long Q)
: _p{p},
_q{Q}
{
if (_q == 0)
throw std::domain_error{"Zero Denominator"};
}
If we use the constructor in the input stream operator, then there's no need for validate() any more, and we've reduced duplication even further:
istream& operator>> (istream& is, Rational& rat)
{
long long p, q;
is >> p >> q;
rat = {p, q};
return is;
}
No need to call simplify() on already-reduced values
All the binary arithmetic operators follow the same (correct) pattern:
Rational Rational::operator+ (const Rational& rat)
{
Rational result(*this);
result += rat;
result.simplify();
return result;
}
The assignment operator (here, +=) has already called simplify(), so there's no need for us to repeat it:
Rational Rational::operator+ (const Rational& rat)
{
return Rational{*this} += rat;
}
But you should call simplify() when constructing:
Rational::Rational(long long p, long long Q)
: _p{p},
_q{Q}
{
if (_q == 0)
throw std::domain_error{"Zero Denominator"};
simplify();
}
If you don't do that, then Rational{1,2} != Rational{2,4} for example.
You can cheat with the overloads of binary operators
The interface makes you overload the binary operators with both const Rational& and long long. But there's a default promotion from long long to Rational so that wasn't really necessary (and isn't any more efficient if the compiler is doing its job properly). We can explicitly construct a Rational to save us repeating ourselves:
Rational Rational::operator+(long long num) const
{
return *this + Rational{num};
}
Rational operator+(long long num, const Rational& rat)
{
return Rational{num} + rat;
}
At least we could, if the header had declared operator+() properly (with const):
Rational operator+(const Rational&) const;
Oddly, the long long overload is declared const, so perhaps that was just to trip you up.
Simplify expressions
Here's a very long-winded way to write a simple test:
bool Rational::operator== (const Rational& rat) const
{
bool result;
if ((this->_p == rat._p) && (this->_q == rat._q))
{
result = true;
}
else
{
result = false;
}
return result;
}
I'd write that without the temporary, as:
bool Rational::operator==(const Rational& rat) const
{
return _p == rat._p && _q == rat._q;
}
Bug: comparison operators
(Conventionally, we implement our comparators in terms of operator> stream operator ignores errors in reading to p and q. This can cause it to try to construct a Rational from uninitialised data. Also, there's a bug, because it doesn't read the separating : that you write (I found this using unit test - see end of answer):
std::istream& operator>>(std::istream& is, Rational& rat)
{
long long p, q;
char sep;
if (is >> p >> sep >> q && sep == ':')
rat = {p, q};
return is;
}
A bug somewhere
The very first test I wrote, failed:
int main()
{
Rational r1{1,4};
Rational r2{2,4};
return (r1 + r2 != Rational{3/4});
}
But this was a bug in the test! Rational{3/4} means Rational{0} (integer division), but I meant Rational{3,4}. I improved the test:
``int verify(Rational actual, Rational expected, const char *expression)
{
if
Code Snippets
Rational::Rational()
{
_p = 0;
_q = 1;
}Rational::Rational()
: _p{0},
_q{1}
{
}Rational::Rational()
: Rational{0}
{
}Rational(long long = 0, long long = 1);Rational::Rational(long long p, long long Q)
: _p{p},
_q{Q}
{
validate(_p, _q);
}
Rational::Rational(const Rational& rat)
: _p{rat._p},
_q{rat._q}
{
}Context
StackExchange Code Review Q#157881, answer score: 19
Revisions (0)
No revisions yet.