2
\$\begingroup\$

This is my solution for the 5th problem of Project Euler, which asks for the least common multiples of the numbers from 1 to 20 (inclusive). Is there any way to improve the while loop conditional instead of using the sum?

table = range(1, 21)
result = 1
pf = 2
while sum(table) > len(table):
   flag = False
   for x, y in enumerate(table):
      if y % pf == 0:
         table[x] = y/pf 
         flag = True
   if flag:
      result *= pf
   else:
      pf += 1
print result
\$\endgroup\$
2
  • \$\begingroup\$ are you interested in a functional solution without loops? \$\endgroup\$
    – tokland
    Commented Jan 30, 2014 at 13:58
  • 1
    \$\begingroup\$ Nah I I know that it's possible to do the same using gcd and reduce, I just want to see how to better optimize this certain algorithm which is basically a pencil and paper algorithm designed for humams. Usually this ends when all elements become 1. \$\endgroup\$
    – Veritas
    Commented Jan 30, 2014 at 16:53

2 Answers 2

2
\$\begingroup\$

Your intent is to terminate the loop when all entries in table become 1. This

while sum(table) > len(table):

is a rather obscure way of expressing that intent. I suggest instead

while max(table) > 1:

or the more explicit and short-circuiting

while any(x > 1 for x in table):
\$\endgroup\$
0
\$\begingroup\$

Use a constant. It's easier to understand when you want to go back and try a different value too.

Copying my answer from stack overflow, also changed things around since this is code review.

MAX_FACTOR = 20 #The largest divisor we want to be able to divide by

#should have a general outline of the algorithm here
table = range(1,MAX_FACTOR + 1)
result = 1 #final result is stored here
currentFactor = 2 #what we're currently trying to divide by
while currentFactor <= MAX_FACTOR:
   isDivisor = False #is the currentFactor a divisor
   for x,y in enumerate(table):
      if y % currentFactor == 0:
     table[x] = y/currentFactor 
     isDivisor = True
   if isDivisor:
      result *= currentFactor
   else:
      currentFactor += 1
print result
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.