Rational arithmetic from given input

Question

I'm currently working on this for my own practice. I get 4 digits and an operation.

The input is x1 y1 op x2 y2 and the fraction is x1/y1 and x2/y2. If I get the input: 1 3 + 1 2 then it's 1/3 + 1/2 and the answer should be given the minimal fractional so it's 5/6. I pass the testcases I get and I can't figure out what I'm doing wrong.

To summarize what I do:

1. Read input and check the operation if it's +,-,/ or *. I generate a prime array to find the biggest common divisor.
2. Send the input to a function depending on which operation it is.
3. I count the given input with simple math.
4. Then I find the biggest common divisor and divide both numerator and denominator with this.
5. After that I print out the result.

Here is the main function and how I handle if the operation is *. I handle the other operation the same but with other math.

 #include <iomanip>
#include <iostream>
#include <vector>
#include <string>
#include <cctype>
#include <iterator>
#include <array>
#include <stdio.h>
#include <string.h>
#include <cstddef>
#include <string>
#include <sstream>
#include <math.h>
#include <cmath>
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <algorithm>
 using namespace std;
long long nwd(long long a, long long b){
 long long c;
 while(b != 0){
 c = a % b;
 a = b;
 b = c;
 }
 return a;
}
void add(long long x1, long long y1, long long x2, long long y2){
 long long bottom = (y1) * (y2);
 long long top = ((x1) * (y2)) + ((x2) * (y1));
 //cout << bottom << " " << top << endl;
 long long frac;
 if(bottom != 0||top != 0){
 frac = nwd(top,bottom);
 }else{
 frac = 1;
 }
 string sign = "";
 if(top * bottom < 0){
 sign = "-";
 }else{
 sign = "";
 }
 printf("%s%lld / %lld\n",sign.c_str(),abs(top/frac),abs(bottom/frac) );
}
void sub(long long x1, long long y1,long long x2, long long y2){
 long long bottom = (y1) * (y2);
 long long top = ((x1) * (y2)) - ((x2) * (y1));
 long long frac;
 if(bottom != 0||top != 0){
 frac = nwd(top,bottom);
 }else{
 frac = 1;
 }
 string sign = "";
 if(top * bottom < 0){
 sign = "-";
 }else{
 sign = "";
 }
 printf("%s%lld / %lld\n",sign.c_str(),abs(top/frac),abs(bottom/frac) );
}
void divi(long long x1, long long y1, long long x2, long long y2){
 long long top = (x1) * (y2);
 long long bottom = (x2) * (y1);
 long long frac;
 if(bottom != 0||top != 0){
 frac = nwd(top,bottom);
 }else{
 frac = 1;
 }
 string sign = "";
 if(top * bottom < 0){
 sign = "-";
 }else{
 sign = "";
 }
 printf("%s%lld / %lld\n",sign.c_str(),abs(top/frac),abs(bottom/frac) );
}
void mult(long long x1, long long y1, long long x2, long long y2){
 long long top = (x1) * (x2);
 long long bottom = (y2) * (y1);
 long long frac;
 if(bottom != 0||top != 0){
 frac = nwd(top,bottom);
 }else{
 frac = 1;
 }
 string sign = "";
 if(top * bottom < 0){
 sign = "-";
 }else{
 sign = "";
 }
 printf("%s%lld / %lld\n",sign.c_str(),abs(top/frac),abs(bottom/frac) );
}
int main()
{
 int numOp;
 scanf("%d", &numOp);
 while(numOp != 0){
 long long x1,x2,y1,y2;
 char op[2];
 scanf("%lld %lld %s %lld %lld", &x1, &y1, op, &x2, &y2);
 if( op[0] == '+'){
 add(x1, y1, x2,y2);
 }
 else if(op[0] == '-'){
 sub(x1,y1,x2,y2);
 }
 else if(op[0] == '/'){
 divi(x1,y1,x2,y2);
 }
 else{
 mult(x1,y1,x2,y2);
 }
 numOp--;
 }
 return 0; 
}

Here is my code with the given testcase and I get the correct result. I need some tips with either different testcases or any suggestions.

• Adam Badura: Use of name nwd for what is called in English GCD (Greatest Common Divisor) suggests you might be from Poland. ;) It is better to use English names for readability.

Answer 1

I see a number of things that may help you improve your code.

Use only required #includes

The code has a great number of #includes that are not needed. Some files are even listed twice. This clutters the code and makes it more difficult to read and understand. Only include files that are actually needed.

Don't Repeat Yourself (DRY)

The mathematical operations all include large portions of repeated code. Instead of repeating code, it's generally better to make common code into a function.

Don't abuse using namespace std

Putting using namespace std at the top of every program is a bad habit that you'd do well to avoid.

Use objects

Since you're writing in C++, what would make more sense to me would be to create a rational fraction object. Then each mathematical operation would be very naturally expressed as an operator on the object.

Use switch instead of nested if

The more appropriate control flow for your if statement in main is a switch statement. This makes it somewhat clearer to read and understand and often provides a small improvement in performance, especially if there are many cases.

Prefer iostream to stdio.h

Using iostream style I/O makes the code easier to adapt to change. For example, using scanf, the code has to have the appropriate matching format string for each argument, but using the >> operator, there is no such requirement and the compiler adapts automatically to whatever type is being read.

Omit return 0

When a C++ program reaches the end of main the compiler will automatically generate code to return 0, so there is no reason to put return 0; explicitly at the end of main.

Putting it all together

Here's an alternative implementation which uses all of these suggestions. Note that a rational fraction is only reduced when it is being printed. This saves some time over always maintaining that state.

#include <iostream>
class RationalFraction
{
public:
 RationalFraction(long long a=0, long long b=1)
 : num{a}, den{b}
 { }
 // unary negation
 RationalFraction& operator-() { num = -num; return *this; }
 // add the passed fraction to this one
 // The two-operand addition is built from this, and a 
 // similar pattern is used for each of the other operators.
 RationalFraction& operator+=(const RationalFraction &rh) {
 num = num * rh.den + rh.num *den;
 den *= rh.den;
 return *this;
 }
 RationalFraction& operator-=(const RationalFraction &rh) {
 num = num * rh.den - rh.num *den;
 den *= rh.den;
 return *this;
 }
 RationalFraction& operator*=(const RationalFraction &rh) {
 num *= rh.num;
 den *= rh.den;
 return *this;
 }
 RationalFraction& operator/=(const RationalFraction &rh) {
 num *= rh.den;
 den *= rh.num;
 return *this;
 }
 friend std::ostream &operator<< (std::ostream& out, const RationalFraction& f) {
 RationalFraction fr=f;
 fr.reduce();
 return out << fr.num << " / " << fr.den;
 }
 friend std::istream &operator>> (std::istream& in, RationalFraction& f) {
 return in >> f.num >> f.den;
 }
private:
 void reduce() {
 if (num == 0) {
 den = 1;
 return;
 }
 long long mygcd = gcd(num, den);
 num /= mygcd;
 den /= mygcd;
 if (den < 0) {
 den = -den;
 num = -num;
 }
 }
 long long gcd(long long a, long long b) const {
 long long c;
 while(b != 0){
 c = a % b;
 a = b;
 b = c;
 }
 return a;
 }
 // data members
 long long num, den;
};
RationalFraction operator+(RationalFraction lh, const RationalFraction &rh)
{
 lh += rh;
 return lh;
}
RationalFraction operator-(RationalFraction lh, const RationalFraction &rh)
{
 lh -= rh;
 return lh;
}
RationalFraction operator*(RationalFraction lh, const RationalFraction &rh)
{
 lh *= rh;
 return lh;
}
RationalFraction operator/(RationalFraction lh, const RationalFraction &rh)
{
 lh /= rh;
 return lh;
}
int main()
{
 int numOp;
 for (std::cin >> numOp; numOp; --numOp) {
 RationalFraction lhs, rhs;
 char op[2];
 std::cin >> lhs >> op >> rhs;
 switch (op[0]) {
 case '+':
 std::cout << lhs+rhs << '\n';
 break;
 case '-':
 std::cout << lhs-rhs << '\n';
 break;
 case '/':
 std::cout << lhs/rhs << '\n';
 break;
 default:
 std::cout << lhs*rhs << '\n';
 }
 }
}

A bit more detail

To understand this version of the code, it's useful to look at a few individual class member functions. First, is the constructor:

 RationalFraction(long long a=0, long long b=1)
 : num{a}, den{b}
 { }

This allows for construction in a few different ways:

RationalFraction half{1,2}; // 1/2 = one half
RationalFraction three{3}; // 3/1 = 3
RationalFraction zero; // 0/1 = 0

The basic four mathematical functions are all built in the same way. First, there is a member function such as +=:

 RationalFraction& operator+=(const RationalFraction &rh) {
 num = num * rh.den + rh.num *den;
 den *= rh.den;
 return *this;
 }

This version is a member function and can be used like this:

RationalFraction a{1,3};
RationalFraction b{1,2}; 
a += b; // now a = 5/6 and b is unchanged

It's a common idiom to define those member functions first and then construct the two-operand versions:

RationalFraction operator+(RationalFraction lh, const RationalFraction &rh)
{
 lh += rh;
 return lh;
}

This is a bit subtle. Note that lh is passed by value, so that means a local copy is constructed using the compiler-generated default copy. By contrast rh is a const reference, so no additional copy is created. Now all we need to do is apply the member function version and return lh which, remember, is a copy of whatever was originally passed in for that parameter. It is a standalone function and not a member function so that either of these works:

RationalFraction f={-1,2};
std::cout << f+3 << std::endl; // (-1/2) + 3 = 5/2
std::cout << 3+f << std::endl; // 3 + (-1/2) = 5/2

In the expression 3+f first a temporary rational fraction is created from 3. Then that rational fraction is added to f and the result displayed using the << operator. That is, it's approximately the equivalent to this:

RationalFraction temp1{3}; // = 3/1
RationalFraction temp2 = temp1 + f; // do addition using non-member function
std::cout << temp2 << std::endl; // show the result = 5/2

The only other slightly tricky bit of code is in private member function reduce. Here I've added comments to make it easier to understand:

void reduce() {
 // if it's of the form 0/x, turn it into canonical 0/1
 if (num == 0) {
 den = 1;
 return;
 }
 // reduce by dividing by the greatest common divisor of 
 // numerator and denominator
 long long mygcd = gcd(num, den);
 num /= mygcd;
 den /= mygcd;
 // Only allow the numerator to be negative to simplify printing.
 // Changing sign of both numerator and denominator has no
 // mathematical effect on the value.
 if (den < 0) {
 den = -den;
 num = -num;
 }
}

• user3664730: Thanks for all the support! Im a beginner at c++ so not really sure how to construct the code yet! Thanks for all the tips.
• user3664730: If you have time can you explain what the different functions in the class does it seem a bit hard for me to understand! Thanks in advance
• Edward: I've added additional explanation to my answer.
• user3664730: Thanks helps me alot! Its very nice and clean code do you have any booktips or anything else to pratice this?
• Edward: Yes, I highly recommend The C++ Programming Language, 4th ed. by Bjarne Stroustrup.

Answer 2

First of all, do you really want to use long long everywhere ? Some simple int aren't enough ? It will really ease the reading to do so.

You really should factorize the + and - operators. You will save tons of line of code and bugs. As you know, x1 y1 sub x2 y2 is equivalent to x1 y1 add -x2 y2. Same for * and / : x1 y1 div x2 y2 is equivalent to x1 y1 mul y2 x2

You never test whether your rationals are valid or not. the bottom of each fractions shall always be non-null, but you never test it. You should add this kind of test case : give invalid values to the main, and see if it crashes, do something wrong, or return an error/exception (that you need to implement).

Have you done tests case where you give non-reduced rationals to your function (like 6/2 + 12/3) ? It should work, and you should test it.

One or two more remarks :

• Prefer the C++ printing (with std::cout) over the C-style printing. You're writing C++, not C (plus, it's clearer and easier to read).
• Don't hesitate to give more explicit names (I don't know what "nwd" stands for...), auto-completion compensate verbosity.
• I always prefer main() parameters over scanf, which is less reliable (and require cautious over flushes). Just read argv ! Moreover, you will probably save lines of code.
• Are all these libs really required ? Anyway, prefer C++ libs over C-converted libs. As I said, you write C++, not C.

That's all I can see for now.