Using separate functions for Project Euler 1

I started programming with Java and C++, so I'm used to having a 'main' function that calls other functions that do the actual work. At university I was always told that doing actual computation in the main function is bad practice. I'm currently playing around with Python, and I have trouble figuring out how to write a nice 'main' function, especially since I'm doing small stuff that doesn't need separate classes.

What do you think about the following code? Is the main function necessary, or would you just write everything without functions? Is there a general consent on this in the Python world?

# Finds sum of all multiples of 3 and 5 from 0 to 999

def find_multiples():
global numbers

for i in range(0,1000):
if i%3==0 or i%5==0:
numbers.append(i);

numbers = []

if __name__ == '__main__':
find_multiples()
print sum(numbers)

In most of the Python code I've ever seen, there is normally a main function defined that you call from the condition you put to establish the script was executed and not imported. I believe that's the standard way of doing things in Python, but I don't believe its actually a written rule anywhere, though that is how they do it in their docs as well.

• Lets state it clearly, what idiom this is, because all comments above are incorrectly formatted and this idiom is not visible: if __name__ == "__main__": (as stated on the page Mark Loeser linked to). Jan 24 '12 at 20:20

Here's some superficial review. More for testing the site and some comment formatting than anything, but: do create main functions (helps us benchmarkers a lot) and do think that your module can be imported, so docstrings and local variables help.

# Finds...
###-^ Put this in a docstring

def find_multiples():
"""Finds sum of all multiples of 3 and 5 from 0 to 999 """
###-^ This allows you to "from x import find_multiples,  help(find_multiples)"
numbers = []
###-^ Avoid globals
for i in xrange(1000):
###-^ Use xrange if Python 2.x, no need to start at 0 (it's the default)
if not (i % 3) or not (i % 5):
###-^ Add spaces around operators, simplify
###-^ the boolean/numeric checks
numbers.append(i)
###-^ Remove trailing ;
return numbers

###-^ Removed global

def main():
###-^ Allows calling main many times, e.g. for benchmarking
numbers = find_multiples()
print sum(numbers)

if __name__ == '__main__':
main()
• xrange(1000) would suffice. Which version is more readable is arguable. Otherwise, excellent code review! Chapeau bas!
– Bolo
Jan 20 '11 at 21:57
• I would be tempted (although perhaps not for such a small program) to move the print statement out of main, and to keep the main() function strictly for catching exceptions, printing error messages to stderr, and returning error/success codes to the shell. Jun 8 '12 at 17:51
• Another reason for using a 'main function' is that otherwise all variables you declare are global. Sep 28 '14 at 14:53

The UNIX Man's recommendation of using a generator rather than a list is good one. However I would recommend using a generator expressions over yield:

def find_multiples(min=0, max=1000):
"""Finds multiples of 3 or 5 between min and max."""
return (i for i in xrange(min, max) if i%3==0 or i%5==0)

This has the same benefits as using yield and the added benefit of being more concise. In contrast to UNIX Man's solution it also uses "positive" control flow, i.e. it selects the elements to select, not the ones to skip, and the lack of the continue statement simplifies the control flow¹.

On a more general note, I'd recommend renaming the function find_multiples_of_3_and_5 because otherwise the name suggests that you might use it to find multiples of any number. Or even better: you could generalize your function, so that it can find the multiples of any numbers. For this the code could look like this:

def find_multiples(factors=[3,5], min=0, max=1000):
"""Finds all numbers between min and max which are multiples of any number
in factors"""
return (i for i in xrange(min, max) if any(i%x==0 for x in factors))

However now the generator expression is getting a bit crowded, so we should factor the logic for finding whether a given number is a multiple of any of the factors into its own function:

def find_multiples(factors=[3,5], min=0, max=1000):
"""Finds all numbers between min and max which are multiples of any number
in factors"""
def is_multiple(i):
return any(i%x==0 for x in factors)

return (i for i in xrange(min, max) if is_multiple(i))

¹ Of course the solution using yield could also be written positively and without continue.

• I like your second version. Perhaps you could make it even more general and succinct by removing the min, max argument, and just letting it take an iterator argument. That way the remaindin program would be sum(find_multiples(xrange(1000))) Sep 28 '14 at 14:55

Here's how I would do it:

def find_multiples(min=0, max=1000):
"""Finds multiples of 3 or 5 between min and max."""

for i in xrange(min, max):
if i%3 and i%5:
continue

yield i

if __name__ == '__main__':
print sum(find_multiples())

This makes find_multiples a generator for the multiples it finds. The multiples no longer need to be stored explicitly, and especially not in a global.

It's also now takes parameters (with default values) so that the caller can specify the range of numbers to search.

And of course, the global "if" block now only has to sum on the numbers generated by the function instead of hoping the global variable exists and has remained untouched by anything else that might come up.

Usually everything in python is handled the come-on-we-are-all-upgrowns principle. This allows you to choose the way you want things to do. However it's best practice to put the main-function code into a function instead directly into the module.

This makes it possible to import the function from some other module and makes simple scripts instantly programmable.

However avoid the use of globals (what you did with numbers) for return values since this makes it difficult to execute the function a second time.

• "...since this makes it difficult to execute the function a second time." Do you mean by this that before calling find_multiples a second time in a main function I would need to empty numbers? Jan 21 '11 at 18:41
• @Basil exactly. Feb 8 '11 at 9:37

Regarding the use of a main() function.

One important reason for using a construct like this:

if __name__ == '__main__':
main()

Is to keep the module importable and in turn much more reusable. I can't really reuse modules that runs all sorts of code when I import them. By having a main() function, as above, I can import the module and reuse relevant parts of it. Perhaps even by running the main() function at my convenience.

Just in case you're not familiar with the generator technique being used above, here's the same function done in 3 ways, starting with something close to your original, then using a list comprehension, and then using a generator expression:

# Finds sum of all multiples of 3 and 5 from 0 to 999 in various ways

def find_multiples():
numbers = []
for i in range(0,1000):
if  i%3 == 0 or i%5 == 0: numbers.append(i)
return numbers

def find_multiples_with_list_comprehension():
return [i for i in range(0,1000) if  i%3 == 0 or i%5 == 0]

def find_multiples_with_generator():
return (i for i in range(0,1000) if  i%3 == 0 or i%5 == 0)

if __name__ == '__main__':
numbers1 = find_multiples()
numbers2 = find_multiples_with_list_comprehension()
numbers3 = list(find_multiples_with_generator())
print numbers1 == numbers2 == numbers3
print sum(numbers1)

find_multiples() is pretty close to what you were doing, but slightly more Pythonic. It avoids the global (icky!) and returns a list.

Generator expressions (contained in parentheses, like a tuple) are more efficient than list comprehensions (contained in square brackets, like a list), but don't actually return a list of values -- they return an object that can be iterated through.

So that's why I called list() on find_multiples_with_generator(), which is actually sort of pointless, since you could also simply do sum(find_multiples_with_generator(), which is your ultimate goal here. I'm just trying to show you that generator expressions and list comprehensions look similar but behave differently. (Something that tripped me up early on.)

The other answers here really solve the problem, I just thought it might be worth seeing these three approaches compared.

I would keep it simple. In this particular case I would do:

def my_sum(start, end, *divisors):
return sum(i for i in xrange(start, end + 1) if any(i % d == 0 for d in divisors))

if __name__ == '__main__':
print(my_sum(0, 999, 3, 5))

Because it is readable enough. Should you need to implement more, then add more functions.

There is also an O(1) version(if the number of divisors is assumed constant), of course.

Note: In Python 3 there is no xrnage as range is lazy.

• Nice and clear. Perhaps don't make an end argument which is included in the range. It's easier if we just always make ranges exclusive like in range. If we do that, we never have to think about what values to pass again :) Sep 28 '14 at 15:00

Adding more to @pat answer, a function like the one below has no meaning because you can NOT re-use for similar tasks. (Copied stripping comments and docstring.)

def find_multiples():
numbers = []
for i in xrange(1000):
if not (i % 3) or not (i % 5):
numbers.append(i)
return numbers

Instead put parametres at the start of your function in order to be able to reuse it.

def find_multiples(a,b,MAX):
numbers = []
for i in xrange(MAX):
if not (i % a) or not (i % b):
numbers.append(i)
return numbers

Here is one solution using ifilter. Basically it will do the same as using a generator but since you try to filter out numbers that don't satisfy a function which returns true if the number is divisible by all the factors, maybe it captures better your logic. It may be a bit difficult to understand for someone not accustomed to functional logic.

from itertools import ifilter

def is_multiple_builder(*factors):
"""returns function that check if the number passed in argument is divisible by all factors"""
def is_multiple(x):
return all(x % factor == 0 for factor in factors)
return is_multiple

def find_multiples(factors, iterable):
return ifilter(is_multiple_builder(*factors), iterable)

The all(iterable) function returns true, if all elements in the iterable passed as an argument are true.

(x % factor == 0 for factor in factors)

will return a generator with true/false for all factor in factors depending if the number is divisible by this factor. I could omit the parentheses around this expression because it is the only argument of all.