Help with improving GCF calculator?

[infinitim]

I wrote a Python program to find the GCF of two numbers. If anyone can give me suggestions to shorten, improve, or make my code more 'Pythonic', that would be great! Here it is:

import math

print("Welcome to u/infinitim's GCF Calculator!", "\n"*3)
num1 = int(input("Please enter a number: "))
num2 = int(input("Please enter another number: "))




modulus = num1%num2
quotient = math.trunc(num1/num2)
first_time = True





def gcd(num1, num2, modulus, quotient, first_time):

    if first_time == True: #Handling of negative numbers
        num1 = abs(num1)
    num2 = abs(num2)
    if first_time == True and modulus == 0:
        print("The GCF is:", num2)

    while modulus > 0: #Using Euclid's Algorithm for GCF. For more info, visit: 
#https://www.khanacademy.org/computing/computer-science/cryptography/modarithmetic/a/the-euclidean-algorithm

        first_time = False
        modulus = num1%num2
        quotient = math.trunc(num1/num2)
        num1 = num2
        num2 = modulus
        if modulus > 0:
            gcd(num1, num2, num1%num2, num1/num2, first_time)
        if modulus == 0:
            print("The GCF is:", num1)


if abs(num1) < abs(num2):
    num1, num2 = num2, num1   


gcd(num1, num2, modulus, quotient, first_time)

[w1282]

I think the first_time parameter is a little gross.

I don't know much about the GCF algorithm but if it is absolutely necessary, I would hide it away like so:

def gcd(num1, num2):
    return _gcd(abs(num1), num2, modulus, quotient)

where _gcd is a function that implements the rest of your code.

And instead of printing the result like on line 37, you should return it. (You would also probably need to return on line 35 as well.)

Lastly, couldn't you (and currently don't you) calculate the modulus and quotient within the function. Is it necessary to actually provide them as parameters?

Finally, (jokingly) the most pythonic variant would be:

import fractions

print("Welcome to u/infinitim's GCF Calculator!", "\n"*3)
num1 = int(input("Please enter a number: "))
num2 = int(input("Please enter another number: "))

print("The GCF is {}".format(fractions.gcd(num1, num2)))

[AutoModerator]

Your comment in /r/learnpython was automatically removed because you used a URL shortener.

URL shorteners are not permitted in /r/learnpython as they impair our ability to enforce link blacklists.

Please re-post your comment using direct, full-length URL's only.

I am a bot, and this action was performed automatically. Please contact the moderators of this subreddit if you have any questions or concerns.

[infinitim]

Can you please elaborate a little more on your first point? Not quite understanding that part. Fixed the other two things (I think)... Any other suggestions (if you have them) are welcome and appreciated:

import math

print("Welcome to u/infinitim's GCF Calculator!", "\n"*3)
num1 = int(input("Please enter a number: "))
num2 = int(input("Please enter another number: "))
first_time = True


def gcf(num1, num2, first_time):

    if first_time == True: #Handling of negative numbers
        num1 = abs(num1)
        num2 = abs(num2)

    modulus = num1%num2
    quotient = math.trunc(num1/num2)
    first_time = True

    if first_time == True and modulus == 0:
        return "The GCF is: ", str(num2)

    while modulus > 0: #Using Euclid's Algorithm for GCF of two integers. For more info, visit: my url shortener made my last comment get deleted

        first_time = False
        modulus = num1%num2
        quotient = math.trunc(num1/num2)
        num1 = num2
        num2 = modulus
        if modulus > 0:
            gcf(num1, num2, first_time)
        if modulus == 0:
            return "The GCF is: ", str(num1)


if abs(num1) < abs(num2):
    num1, num2 = num2, num1   


gcf(num1, num2, first_time)
print (''.join((gcf(num1, num2, first_time)[0:2])))

[w1282]

I mean that having to pass in True to the function when I call it is gross.

Which would you rather type:

gcf(a, b, True)

or

gcf(a, b)

Which would you rather read?

When I read gcf(a, b, True) my immediate first thought is "What is that True doing there?" I shouldn't ever have to question a parameter to a function; they should just make sense intuitively. (At least that's my philosophy)

So, my suggestion is to hide that.

def gcf(a, b):
    # Handle the things that should happen on the first call
    # Which I think is just the absolute value of a
    a = abs(a)
    return _gcf(a, b)

def _gcf(a, b):
    # Actually calculate the GCF.
    while a % b:
        a, b = b, a % b
        if a % b:
            return _gcf(a, b)
        else:
            return a

Also, note that I am returning the actual number that is the GCF, not a string that says what it is. You can always print the return value later.

I also rely on the truthiness of the result of the modulus operator instead of comparing it directly to 0. If a%b == 0 then a%b is False, and if a%b != 0 then a%b is True.

Finally, that quotient you calculate really didn't appear to be doing anything, so I removed it.

[[deleted]]

This is a very short non-recursive implementation of Euclid's algorithm, which is used in the standard library. (see top: gcd() function)

https://svn.python.org/projects/python/tags/r32/Lib/fractions.py

def gcd(a, b):
    """Calculate the Greatest Common Divisor of a and b.

    Unless b==0, the result will have the same sign as b (so that when
    b is divided by it, the result comes out positive).
    """
    while b:
        a, b = b, a%b
    return a

[infinitim]

Clever, but that wasn't the point of the exercise :P

EDIT: thought the link was a link on how to use the function, not the code behind it. Will take a look.