patterncppMajor
Recursive and iterative Euclidean algorithms
Viewed 0 times
iterativeeuclideanalgorithmsrecursiveand
Problem
Is this code too cryptic? Simple yes or no question. Feedback is optional.
int recursive_euclidean( int num1, int num2 )
{
int gcd=0;
if( num1 == num2 || num1 num2 ) ? ( gcd = recursive_euclidean( num1-num2, num2 ) ) :
( gcd = recursive_euclidean( num1, num2-num1 ) ) );
return gcd;
}
int iterative_euclidean( int num1, int num2 )
{
while( num1 != num2 && num1 > 0 && num2 > 0 )
(( num1 > num2 ) ? (num1-=num2) : (num2-=num1) );
return (num1>0) ? num1 : num2;
}Solution
I don't like the multiple different assignments inside the operator.
They currently all assign to
I would make the assignment explicit and the ternary operator return the correct value.
The other thing with ternary operator is that they are not trivial to read. So use white space to try and make it obvious that it's happening. Also I prefer to explicitly use '{' '}' to make sure that things don't accidentally become associated with the a different statement. I would lay it out like this:
Actually reading this again I would refactor more to this:
Then here, I would not personally not use the ternary operator.
They currently all assign to
gcd but you need to study it in detail to work that out.I would make the assignment explicit and the ternary operator return the correct value.
The other thing with ternary operator is that they are not trivial to read. So use white space to try and make it obvious that it's happening. Also I prefer to explicitly use '{' '}' to make sure that things don't accidentally become associated with the a different statement. I would lay it out like this:
int recursive_euclidean( int num1, int num2 )
{
int gcd;
if( num1 == num2 || num1 num2)
? recursive_euclidean( num1-num2, num2 )
: recursive_euclidean( num1, num2-num1 );
}
return gcd;
}Actually reading this again I would refactor more to this:
int recursive_euclidean( int num1, int num2 )
{
int gcd;
if ( num1 == num2)
{ gcd = num1;
}
else if (num1 num2)
? recursive_euclidean( num1-num2, num2 )
: recursive_euclidean( num1, num2-num1 );
}
return gcd;
}Then here, I would not personally not use the ternary operator.
int iterative_euclidean( int num1, int num2 )
{
while( num1 != num2 && num1 > 0 && num2 > 0 )
{
if (num1 > num2 )
{ num1-=num2;}
else{ num2-=num1;}
}
return (num1>0) ? num1 : num2;
}Code Snippets
int recursive_euclidean( int num1, int num2 )
{
int gcd;
if( num1 == num2 || num1 < 0 || num2 < 0 )
{
gcd = (num1 == num2)
? num1
: (num1 < 0)
? num2
: num1;
}
else
{
gcd = (num1 > num2)
? recursive_euclidean( num1-num2, num2 )
: recursive_euclidean( num1, num2-num1 );
}
return gcd;
}int recursive_euclidean( int num1, int num2 )
{
int gcd;
if ( num1 == num2)
{ gcd = num1;
}
else if (num1 < 0)
{ gcd = num2;
}
else if (num2 < 0)
{ gcd = num1;
}
else
{
gcd = (num1 > num2)
? recursive_euclidean( num1-num2, num2 )
: recursive_euclidean( num1, num2-num1 );
}
return gcd;
}int iterative_euclidean( int num1, int num2 )
{
while( num1 != num2 && num1 > 0 && num2 > 0 )
{
if (num1 > num2 )
{ num1-=num2;}
else{ num2-=num1;}
}
return (num1>0) ? num1 : num2;
}Context
StackExchange Code Review Q#3955, answer score: 21
Revisions (0)
No revisions yet.