Recursive and iterative Euclidean algorithms

Question

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 < 0 || num2 < 0 ) 
        ( ( num1 == num2 ) ? ( gcd = num1 ) : ( ( num1 < 0) ? ( gcd = num2 ) : 
            ( gcd = num1 ) ) );
    else
        ( ( 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;
}

Answer 1

I don't like the multiple different assignments inside the 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 < 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;
}

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 < 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;
}

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;
}

Answer 2

Why do you use || between the three conditions only to then make a decision about the conditions seperately? Why don't you write it like this:

int recursive_euclidean( int num1, int num2 )
{
    if( num1 == num2 )
        return num1;
    else if( num1 < 0 )
        return num2;
    else if( num2 < 0)
        return num1;
    else if( num1 > num2) 
        return gcd = recursive_euclidean( num1-num2, num2 ) ) :
    else
        return recursive_euclidean( num1, num2-num1 ) ) );
}

I hereby ban you from use of the ternary operator due to egregious abuse. Not seriously, but its not clear why you are trying to bring it in here.

Answer 3

I think the simple test would be to have an outsider read the code and understand what it does. I did just that, and its perfectly readable to me. Actually, I find it to be a perfect mix of readability and compactness.

Answer 4

I just like this:

int recursive_euclidean( int num1, int num2 )
{
    return    num1 == num2 ?  num1  
            : num1 < 0     ?  num2
            : num2 < 0     ?  num1
            : num1 > num2  ?  recursive_euclidean( num1-num2, num2 )
            :                 recursive_euclidean( num1, num2-num1 );
}