how to improve - HCF

[MinwooJu]

#include<iostream>
using namespace std;

void hcf(int a,int b) {
    

const int num1 = a;
const int num2 = b;
int c;
int hcf;
bool check_hcf = false;
bool fail = false;

while(check_hcf == false) {
      


          
if(a > b)
c = a % b;

else  {
cout << "a must be bigger than b." << endl;
check_hcf = true;
fail = true;
}

if( c == 0) {
    
hcf = b;
check_hcf = true;

}

else {
a = b;
b = c;
}       
}    

if(fail == false) {
cout << "the HCF of " << num1 << " and " << num2 << " is:" << endl; 
cout << hcf << endl;
}

else {
     
cout << "the function failed.Sorry!" << endl;
}
}

int main() {

int a,b;

cout << "type in two numbers to calculate the HCF with." << endl;
cin >> a >> b;
                                                                  
hcf(a,b);


cin.get();
cin.get();
return 0;
}

improvement needed - HCF program.

This program is done inspired by the Euclid's algorithm. What can I improve?

[vlad from moscow]

I have a question: does HCF mean the same as GCD?

As for the code then it is very bad. For example why does the name of variable
(int hcf;) coinside with the name of the function? Why was not the condition if(a > b) done before the loop?! And so on.

[MinwooJu]

yep. We call it 'Highest common factor'.

#include<iostream>
using namespace std;

void hcf_calculator(int a,int b) {
    

const int num1 = a;
const int num2 = b;
int c;
int hcf;
bool check_hcf = false;
bool fail = false;

if (a > b) {

while(check_hcf == false) {
      


          

c = a % b;



if( c == 0) {
    
hcf = b;
check_hcf = true;

}

else {
a = b;
b = c;
}       
}



    

if(fail == false) {
cout << "the HCF of " << num1 << " and " << num2 << " is:" << endl; 
cout << hcf << endl;
}
}

else {
     
cout << "the function failed.Sorry!" << endl;
}
}

int main() {

int a,b;

cout << "type in two numbers to calculate the HCF with." << endl;
cin >> a >> b;
                                                                  
hcf_calculator(a,b);


cin.get();
cin.get();
return 0;
}

-made the varible names unique.
-the condition is set outside the loop.

happy now?

[vlad from moscow]

No, the code is bad. The function shall return HCF. If somebody want to get HCF he should get what he want.

[MinwooJu]

okay.So I should make it a 'int' function.Is that what you're saying? but what if the user has entered a bad input? how am I going to make the function handle that?

[vlad from moscow]

What is a bad input? He supplies two integers and you shall return HCF for these two integers. What is the problem?

By the way if one of the integers is equal to zero what will be the HCF by mathematical definition of the HCF?

[MinwooJu]

Euclid's algorithm states that in order for the HCF to be calculated, a should be bigger than b, and Euclid's algorithm also suggests that-

divide a by b if a is bigger than b,and call the remainder c.
if c is zero, then b is the HCF.

if it's not zero, you divide b by c and call it d. Then you carry on until you get the remainder that is zero, and if you get a remainder that is zero then the remainder before it must be the HCF.

this is what the whole program is based on. What if a is smaller than b? if HCF is a interger function, how are we going to handle that error?

[vlad from moscow]

I have not got the answer what is HCF if one of two integers is equal to zero.

I have prepared a simple code, look through it

#include <iostream>
#include <algorithm>

int HCF( int a, int b )
{
	if ( a < b ) std::swap( a, b );

	while ( b )
	{
		int tmp = a % b;
		a = b;
		b = tmp;
	}

	return ( a );
}


int main()
{
	std::cout << HCF( 15, 3 ) << std::endl;
	std::cout << HCF( 4, 2 ) << std::endl;
	std::cout << HCF( 3, 1 ) << std::endl;
	std::cout << HCF( 5, 5 ) << std::endl;
}

[MinwooJu]

std::swap huh?

so that was a way to swap a and b if a < b!
gee,I learnt a new thing.

*p.s - GCD() is supposed to be HCF() btw :D

[AbR]

It doesn't work if one interger is negative (it's ok if they both are).

But if you accept that the input should be different from zero and either both negative or both positive something like this might lead you in the right direction.

(You really should check if the user input is indeed valid for the function instead of supposing the user will know what is valid)

Yeah, you should forget what was here, sorry 'bout that.

[vlad from moscow]

@AbR,

it seems that your loop is invalid

for (c=a%b; c>0; c--)

[MinwooJu]

@AbR

unfrotunately, your program has problems. :D

[vlad from moscow]

@MinwooJu.

as AbR has pointed out it would be a good idea to convert parameters to non-negative values if they have negative values.

[MinwooJu]

would I need to multiply them by -1 with 'if' statements if I locate a negative value? or is there a smarter way of doing so?

[vlad from moscow]

if ( a < 0 ) a = -a;
if ( b < 0 ) b = -b;
if ( a < b ) std::swap( a, b );

[MinwooJu]

thought so. :D

[MinwooJu]

can't you also do -

 if (a < 0 ) a *= -1;
 if (b < 0 ) b *= -1;
if ( a < b ) swap( a, b );

[vlad from moscow]

It is simply to use a = -a; though maybe compilers generate the same mashine code for a *= -1; But in any case the multiplication looks worse.

[MinwooJu]

yeah,it does look untidy. Thank you. I think you told me alot of things today.

[ne555]

There is no need to if ( a < b ) std::swap( a, b );
The algorithm will get the invariant in the first iteration.

As for zero, all numbers are factors of zero, so gcd(0,x) = x \forall x \neq 0

gcd(0,0) would be undefined, but in this special case you could use the error code 0 (that's what the algorithm returns), as it can't happen with another pair.

As for negatives, I think that x = abs(x); is clearer.