HiveBrain v1.2.0
Get Started
← Back to all entries
patternpythonMinor

Simple GCF calculator app

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
simplecalculatorappgcf

Problem

I am trying to learn some basic Python and am trying to find tricks to write speedy code:

import sys
if len(sys.argv) < 3:
    print("You left out the arguments")
    sys.exit()
gcf = 0

numbers = []
collections = []
unique = False

for i in range(1,len(sys.argv)):
    numbers.append(int(sys.argv[i]))
for x in range(0,len(numbers)):
    collections.append([])  
    for s in range(1,numbers[x] + 1):
        if numbers[x] % s == 0:
            collections[x].append(s)
    print str(numbers[x]) + ": ",       
    print ','.join(map(str,collections[x]))
for i in collections[0]:
        for x in collections:
            if i in x:
                unique = True
            else:
                unique = False
        if unique:
            gcf = i
print "gcf is: "+str(gcf)

Solution

You can use sets to whittle down the possible GCFs.

for i, argument in enumerate(sys.argv[1:]):
    factors = set(n for n in range(2, int(argument) + 1) if int(argument) & n is 0)
    print argument, factors
    if i is 0:
        possible_gcfs = factors
    else:
        possible_gcfs &= factors
print "gcf is:", max(possible_gcfs | {1})


Better, use reduce:

def get_factors(x):
    factors = set(n for n in range(2, int(x) + 1) if int(x) % n is 0)
    print x, ':', factors
    return factors

possible_gcfs = reduce(set.intersection, map(get_factors, sys.argv[1:]))
print "gcf is:", max(possible_gcfs | {1})


Or if you don't need it to print out the factors of each number you can make it a bit more efficient by noting that the GCF must be no larger than the lowest of the arguments entered.

arguments = map(int, sys.argv[1:])
possible_gcfs = range(2, min(arguments) + 1)
for argument in arguments:
    possible_gcfs = [n for n in possible_gcfs if argument % n is 0]
print "gcf is: ", max(possible_gcfs + [1])


... but this can again be simplified using reduce and noting that the gcf is associative

def gcf(a, b):
    a, b = int(a), int(b)
    return max(n for n in range(1, min(a, b) + 1) if a % n is 0 and b % n is 0)

print "gcf is:", reduce(gcf, sys.argv[1:])


(And as a comment pointed out there are plenty of other solutions posted to earlier questions and you could use fractions.gcd.)

Code Snippets

for i, argument in enumerate(sys.argv[1:]):
    factors = set(n for n in range(2, int(argument) + 1) if int(argument) & n is 0)
    print argument, factors
    if i is 0:
        possible_gcfs = factors
    else:
        possible_gcfs &= factors
print "gcf is:", max(possible_gcfs | {1})
def get_factors(x):
    factors = set(n for n in range(2, int(x) + 1) if int(x) % n is 0)
    print x, ':', factors
    return factors

possible_gcfs = reduce(set.intersection, map(get_factors, sys.argv[1:]))
print "gcf is:", max(possible_gcfs | {1})
arguments = map(int, sys.argv[1:])
possible_gcfs = range(2, min(arguments) + 1)
for argument in arguments:
    possible_gcfs = [n for n in possible_gcfs if argument % n is 0]
print "gcf is: ", max(possible_gcfs + [1])
def gcf(a, b):
    a, b = int(a), int(b)
    return max(n for n in range(1, min(a, b) + 1) if a % n is 0 and b % n is 0)

print "gcf is:", reduce(gcf, sys.argv[1:])

Context

StackExchange Code Review Q#29464, answer score: 3

Revisions (0)

No revisions yet.