2
\$\begingroup\$

I have created a rationalnumber class and used operator overloading function of C++.

My questions are:

  1. Have I implemented the class and < operator correctly?
  2. Objects of class can now inserted in map and set. Is that part done correctly?

#include <bits/stdc++.h>
using namespace std;

class RationalNumber {
    int num;
    int den;
    bool negative;
    int gcd(int a, int b) {
        if (b == 0)
            return a;
        return gcd(b, a % b);
    }
public:
    RationalNumber() {
        num = den = 1;
        negative = 0;
    }
    RationalNumber(int a, int b) {
        if ((a < 0) ^ (b < 0))
            negative = 1;
        else
            negative = 0;
        num = abs(a);
        den = abs(b);
        int h = gcd(a, b);
        num /= h;
        den /= h;
    }
    RationalNumber operator+(const RationalNumber &r) {
        RationalNumber added;
        added = RationalNumber(num * r.den + r.num * den, den * r.den);
        return added;
    }
    bool operator<(const RationalNumber &r) const {
        return num * r.den < r.num * den;
    }

    void toString() {
        cout << num << "/" << den << endl;
    }
};
int main(int argc, char **argv) {
    RationalNumber r1(3, 5);
    RationalNumber r2(1, 3);
    RationalNumber r3 = r1 + r2;
    RationalNumber r4(9, 13);
    vector<RationalNumber> v;
    map<RationalNumber, string> S;
    map<RationalNumber, string>::iterator it;
    S[r1] = "1";
    S.insert(S.begin(), pair<RationalNumber, string>(r2, "2"));
    S.insert(S.begin(), pair<RationalNumber, string>(r2, "2"));
    S.insert(S.begin(), pair<RationalNumber, string>(r3, "3"));
    S.insert(S.begin(), pair<RationalNumber, string>(r4, "4"));
    for (it = S.begin(); it != S.end(); ++it) {
        RationalNumber r = it->first;
        r.toString();
        cout << it->second << endl;
    }
    return 0;
}
\$\endgroup\$

closed as off-topic by Dannnno, Edward, ferada, Jamal Aug 23 '16 at 3:26

This question appears to be off-topic. The users who voted to close gave this specific reason:

  • "Questions containing broken code or asking for advice about code not yet written are off-topic, as the code is not ready for review. After the question has been edited to contain working code, we will consider reopening it." – Dannnno, Edward, ferada, Jamal
If this question can be reworded to fit the rules in the help center, please edit the question.

  • \$\begingroup\$ Have you verified whether it works as intended? \$\endgroup\$ – Mast Aug 22 '16 at 7:52
  • \$\begingroup\$ @Mast I ran few test cases it is working fine \$\endgroup\$ – Prashant Bhanarkar Aug 22 '16 at 8:25
  • \$\begingroup\$ Then you don't need to ask if it's working correctly. We can only review this code if it really does work. \$\endgroup\$ – Jamal Aug 22 '16 at 10:18
  • \$\begingroup\$ Since you support negative numbers, add some test cases for them. Also check and see what 1/2+1/2+1/2 prints? \$\endgroup\$ – ilkkachu Aug 22 '16 at 10:31
  • \$\begingroup\$ @ilkkachu, it prints 3/2. It should be right. \$\endgroup\$ – Incomputable Aug 22 '16 at 10:40
2
\$\begingroup\$

Style:

  • Use an initialiser list for constructors, with meaningful parameter names. example at the end.

  • Please use the right type for negative - true/false makes it a lot easier to read and understand inline what type negative is.

  • Naming: The toString() is easier expressed by returning a std::string - that can be used as-is or forwarded to std::cout or other logs. If you want to use a log output, (e.g. std::cout) it can be trivially implemented using a template operator<< with a generic ostream input - in this case it is not required (see below)

  • The operator+ can be expressed in a single line, without the temporary added variable.

  • it is desirable to make gcd an non-member file-local static function (static gcd(int a, int b)) the rationale being that the logic:

    • doesn't depend on the state (or const-ness) of the Rational object (so non-member)
    • doesn't need to access the (private/protected) innards of Rational(so non-static, non-friend)
  • In main, the for loop can be shortened (in general, if a function call chain helps avoid a temp variable, do so). example at the end.

The same logic (avoiding a temp variable) holds for defining r3 and r4 inlined when adding them to the map.

  • vector v is unused.

  • Include what you use: vector, map, iostream, cmath (and iterator if you decide to use ostream_iterator.

  • maps don't require an input iterator during insertion. Use std::make_pair(a, b) to cut down on the verbose template boilerplate.

Logical Issues:

  • Translation into abs and negative is done in the ctor and not followed up well in other places. This will fail for negative test cases. hint: why is negative required as a member?

  • adding 1/4 and 1/2 results in 6/8 - while this isn't incorrect, this defeats the purpose. Invoke gcd there.

  • Depending on how you want your object to behave, handle the case of (den == 0), either by explicity asserting against its occurence, or by adding special handling for arithmetic operators. You don't want a 1/0 + 5/2 == 2/0 comparison.

Constructor style preferred:

RationalNumber() : num(1), den(1), negative(false) {}
RationalNumber(int numerator, int denominator) 
: num(abs(numerator))
, den(abs(denominator))
, negative((numerator < 0) == (denominator < 0)) 
{
    //use gcd here
}

Terser loop style suggestions:

for (map<RationalNumber, string>::iterator it = S.begin(); it != S.end();++it) 
{
    std::cout << "comparing:" << itr->first.toString() << itr->second << std::endl;
}

or the even terser lambda:

transform(S.begin(), S.end(), ostream_iterator<string>(cout, '\n'), 
    [] (const pair<RationalNumber, string>& p) { return p.first.toString() + " " + p.second; });

edits: Moved code examples to the end - as code indents don't work in between bulleted points.

\$\endgroup\$
  • 1
    \$\begingroup\$ ironically, I can't seem to get my code-formatting right (despite the correct indents). I'd appreciate if someone told me where my formatting went wrong and what I should do about it the next time. \$\endgroup\$ – Fox Aug 23 '16 at 6:15
0
\$\begingroup\$

I didn't check the correctness of your code, but I have a few comments about it. Anyway, you should write a few unit tests to test your classes and functions.

Here are a few suggestions and possible improvements for your code. I'm assuming that you're compiling with C++11 or higher.

  1. Including bits/stdc++.h seems like an overkill. It results in the inclusion of many (not sure if all) libraries from the C++ standard, even though you use just a few of them. So I would replace it with an inclusion of isotream, vector, etc. This will reduce compile time and program size.
  2. "using namespace" is usually considered bad practice. You should only take the parts of std that you need, i.e.

    using std::vector;
    using std::map;
    // and so on...
    
  3. You should declare int gcd(int, int) as a static function, since it doesn't require a class object as an argument. This will also instruct the compiler to create a single gcd function for all objects of type RationalNumber, rather than create multiple instances of the same function (one in each object of the class). Declaring a function to be static is done as follows:

    static int gcd(int a, int b) {
        // insert code here
    }
    
  4. Use instantiation lists when possible. Instead of your default constructor you could write: RationalNumber() : num(1), den(1), negative(0) { } (the body is empty, because there is nothing left to be done.). The same point applies to your second constructor.

  5. You can "merge" both of your constructors into one by using C++'s default arguments feature. The result is as follows:

    RationalNumber(int a = 1, int b = 1) : negative(0) { if ((a < 0) ^ (b < 0)) // more code here } The statements RationalNumber() and RationalNumber(a, b) will still work the same.

  6. A good chunk of your code can be simplified andmade shorter. For example, the addition function can be written as follows:

    RationalNumber operator+(const RationalNumber &r) { return RationalNumber(num * r.den + r.num * den, den * r.den); }

  7. The toString function can be replaced by an overloading of the << operator. Here's one way of doing this, using friend:

    friend std::ostream& operator<<(std::ostream& os, const RationalNumber& r) {
        return os << r.num << '/' << r.den << endl;  
    }
    

    You can then just use cout << r to print an object of the class in the format above. I should mention that the use of friend is considered bad practice by some users of the language. It can be avoided here, but using friend is still the simplest implementation in this case in my opinion.

  8. Instead of overloading the + operator the way you did, you can overload the += operator for RationalNumber and then implement operator+ as a nonmember function via +=. This is considered better practice; The implementation of operator+ is then more symmetric.

  9. The statements RationalNumber r3 = r1 + r2; and RationalNumber r = it->first; require a copy constructor for RationalNumber. Since you didn't define one, the compiler uses a default copy constructor, which simply copies the value of each field in RationalNumber. This works fine in this case, but I would also add the following decleration to the class:

    RationalNumber(const RationalNumber&) = default;
    

    This way it's clear that you intended to use the default copy constructor, and didn't just forget to write a specialized copy constructor;

\$\endgroup\$
  • 1
    \$\begingroup\$ I downvoted for 5 reasons: 1. iostream.h, vector.h. 2. using std::cout and using std::endl. 3. Very strange statement about gcd() function without any justification. 4. No mention to avoid std::endl. 5. Bad formatting of the code. For #5 please use 4 spaces after newline to make it a code block (or just select it and hit ctrl+k). Making gcd() static is not good idea in my opinion, but it is arguable decision. \$\endgroup\$ – Incomputable Aug 22 '16 at 12:43
  • \$\begingroup\$ I am curious: What is bad about gcd() being static? \$\endgroup\$ – Martin R Aug 22 '16 at 13:48
  • 1
    \$\begingroup\$ @MartinR, that's why I said it is arguable :) I think it is pretty useful to stand on it's own, so may be putting it with other useful algorithms into it's own namespace, and then just invoking it. \$\endgroup\$ – Incomputable Aug 22 '16 at 17:15
  • \$\begingroup\$ @OlzhasZhumabek: 3. Since gcd is a private member function, it can't be used outside the class. 4. The usage of std::endl is quite common in the C++ code I've come across. I know that '\n' is preferable in some respects but in this case it doesn't matter much which of these you use. 5. Fair point. This is my first post in CR, so I'm still not familiair with all of the formatting options here. I edited my answer to try and make it look nicer. Apologies. I didn't understand your objections (1) and (2). Can you elaborate on them? \$\endgroup\$ – Mark Aug 23 '16 at 9:36
  • \$\begingroup\$ @Mark, I apologies for being a little rude. I think you assume that functions are objects in C++. They are not (It is explicitly mentioned by the C++ standard). The difference between std::endl and '\n' is that the first flushes the buffer and puts newline, which may have impact on performance. for #1, iostream is C++ header, and everyone is used to that. .h files make people think that it is either C header or user created header. You have only very few cases where you want to write usng xxx;. Swap expression is one of them. \$\endgroup\$ – Incomputable Aug 23 '16 at 9:54

Not the answer you're looking for? Browse other questions tagged or ask your own question.