make it more efficient

[John3682]

guys can someone make my code more efficient like to make it run faster ?
specially the main , i know the user is going to enter 4 num and den but what if the user wanted to enter more or less , can u make the code compatible ?
or the simplify function [simp] , how can i use it once instead of writing it multiple time ?
write it with operators if possible
ty in advance : )

#include <iostream>
using namespace std;
int gcd(int, int);
class rational 
{
	int num, den;
public :
	rational(int num = 0, int den = 1) {}
	void Fsum(int n, int d, int n1, int d1) { num = ((n * d1) + (n1 * d)) ; den = (d*d1); }
	void Fsub(int n, int d) { num = ((num * d) - (den * n)); den *= d; };
	void Fdiv(int n3, int den3) { int r; r = n3; n3 = den3; den3 = r; num *= n3; den *= den3; };
	void Fmul(int n2, int d2) { num *=  n2; den *= d2; };
	void print() { cout << num << "\\" << den << "\n"; float n = num, d = den;  cout << (n / d) << "\n"; }
	friend void simp(rational&);
};
void simp(rational &R) 
{
		int F = gcd(R.num, R.den);
		R.num = R.num / F;
		R.den = R.den / F;
}
int gcd(int a, int b) {
	if (b == 0){
		return a;
	}
	return gcd(b, a % b);
}
void fill(int* n, int* d , bool& imp) {
	imp = true;
	for (int i = 0; i < 4; i++)
	{
		cin >> n[i] >> d[i];
		if (d[i] == 0)
		{
			imp = false;
		}
	}
}
int main()
{
	rational R;
	bool imp;
	int n[4], d[4];
	fill(n, d , imp);
	if (imp == false)
	{
		cout << "0\\0\n"<<"impossible";
		return 0;
	}
	R.Fsum(n[0],d[0],n[1],d[1]);
	simp(R);
	R.Fmul(n[2],d[2]);
	simp(R);
	R.Fdiv(n[3],d[3]);
	simp(R);
	R.Fsub(n[0], d[0]);
	simp(R);
	R.print();
}

[jonnin]

you can't skip the simplify calls. You need one any time num or denom changes if you want the reduced answer.
gcd is part of c++ now, no need to hand write one, use the (probably faster) language one.

before optimize, maybe clean it up...
why don't you overload operators to add & subtract etc instead of the wonky functions?
use double instead of float unless there is a reason to use float.

your current code probably spends more time waiting on the user to type in the values than it does processing.

[John3682]

when i try to use :

__gcd(a,b)

it says identifier "__gcd" is undefined

[George P]

identifier "__gcd" is undefined

Try std::gcd(), it was added in C++17.
https://en.cppreference.com/w/cpp/numeric/gcd

[Handy Andy]

Hello John3682,

That is because all you need is gcd(a,b);.

The __gcd(a,b) is what the header files used and is not meant for normal use.

See https://en.cppreference.com/w/cpp/numeric/gcd

Andy

[dhayden]

Why is simp() a friend instead of a method?

If you haven't learned about overloading operators, at least make the functions use rational objects.

You should probably call simp() inside the +,-,*,/ operations

If the numerator is negative, simp should multiply numerator and denominator by -1.
Since the denominator could be negative, simp should call gcd(num, abs(den));

[John3682]

changed simp from friend to a method as u said :

void simp() {int F = gcd(num, abs(den)); num = num / F; den = den / F;}

also is there a reason why i had to do that ?

----------------------------------------------------
but still have problem with the gcd (standard not the one i wrote) . i cant use the inbuilt gcd.

i used

gcd (5,6)

but still didnt work

ty all for helping : )

[George P]

cant use the inbuilt gcd.

What is your compiler? It might not be C++17 capable.

i used gcd (5,6) but still didnt work

Saying "it doesn't work" is not helpful. For you and us. If using std::gcd has compile-time errors telling us what the errors are would help us be able to help you.

[John3682]

im using visual studio 2019

#include <iostream>
using namespace std;

int main()
{
	cout << gcd(6, 12);
}

identifier "gcd" is undefined
'gcd' : identifier not found

[Ganado]

You need to #include <numeric>

But Visual Studio still might be using C++14 by default, so if it is, you need to specify the /std (standard) used.
Please see: https://docs.microsoft.com/en-us/cpp/build/reference/std-specify-language-standard-version?view=vs-2019

I don't know why all this talk about gcd happened. Yes, C++ has a definition built-in, but your original implementation looks fine. I think we're going down the wrong rabbit hole. The original question was about efficiency; the gcd implementation here can't get much better.

[George P]

Visual Studio still might be using C++14 by default

C++14 is the default for VS2019, so the project/solution settings need to be altered to use C++17 or later.

[jonnin]

my fault. I made an assumption that gcd would be slightly faster done in assembly, which it would be in the language version on a good compiler.

conditions (or, in assembly/cpu terms, jumps) create sluggishness sometimes (its difficult to say when it will or won't due to hardware and compiler magic) so getting rid of unnecessary branches "can" help.

your:
if (d[i] == 0)
{
imp = false;
}

favor:
imp = (bool) d[i];

[dhayden]

favor:
imp = (bool) d[i];

That won't quite work because it could set imp=false on iteration 3 and then set it back to true on iteration 4. You could do imp = imp && d[i], but that's likely to involve a branch due to the short-circuit &&. Here's a trick that avoids the branch. I'll include the whole function:

void fill(int* n, int* d , bool& imp) {
	int flag = -1;
	for (int i = 0; i < 4; i++)
	{
		cin >> n[i] >> d[i];
		flag &= (d[i] | -d[i]);
	}
	imp = flag;
}

If d[i] is non-zero then either d[i] is negative or -d[i] is negative. So d[i] | -d[i] is guaranteed to have the sign bit set. If d[i] is zero then d[i] | -d[i] is zero. So if d[i] is ever zero, flag will be set to 0. Otherwise it will remain non-zero.

[jonnin]

good catch.
maybe, an alternate?
imp = true;
...
imp &= (bool)d[i]; //now if it goes false, it stays false.