3
\$\begingroup\$

I have decent experience in programming before (mostly C++), but I am very, very, very new to Python, and I decided to try out Project Euler as an exercise. Here's the description of Problem 1:

If we list all the natural numbers below 10 that are multiples of 3 or 5, we get 3, 5, 6 and 9. The sum of these multiples is 23.

Find the sum of all the multiples of 3 or 5 below 1000.

Here's my solution:

sum = 0
for n in range(0, 1000):
    if n % 3 == 0 or n % 5 == 0:
        sum += n
print(sum)

The program works fine. What I want to get from a code review:

  • whether I did anything non-Pythonic;

  • whether I did anything outdated;

  • whether I did anything against the PEP 8;

  • whether there's a better way of doing this;

  • ...

\$\endgroup\$
  • \$\begingroup\$ Oops, I got a downvote in 40 seconds :( Did I do something wrong? \$\endgroup\$ – L. F. Sep 7 at 11:52
  • \$\begingroup\$ hint: the sum of 1.to x is (x*(x+1))/2. I'm pretty sure there's a way to use it here too \$\endgroup\$ – Tommylee2k Sep 18 at 13:51
2
\$\begingroup\$

Welcome to Python!

"Project Euler exists to encourage, challenge, and develop the skills and enjoyment of anyone with an interest in the fascinating world of mathematics."

Like you, I went to Project Euler when I was learning Python as yet another language for my toolbox. Unfortunately, Project Euler is primarily a mathematics challenge site, not a programming challenge site. The emphasis is on solving the problem and getting the right answer; not on programming skills. More over, the site asks that you not post your solution, which really discourages you from getting feedback on your programming skills, and proper best practices for the language. So while you can get problems that you can actually try writing code to solve the problem for, you’re still discouraged from getting feedback on your approach. Not exactly an ideal site for learning a new language on.

Still, you have violated their request and posted your solution, so let’s try and give you some useful feedback.

Write Functions

You’ve got two sets of data to try your solution on. The first being the “example” data in the problem itself; the second being the dataset you are being asked to solve. If you’re going to do something twice, write a function:

def sum_of_multiples_of_3_or_5_below(limit):
    total = 0
    for n in range(0, limit):
        if n % 3 == 0 or n % 5 == 0:
            total += n
    return total

Then you can test your function with the example data, as well as solve the problem:

assert sum_of_multiples_of_3_or_5_below(10) == 23

answer = sum_of_multiples_of_3_or_5_below(1000)
print(f"sum of all multiples of 3 or 5 below 1000 is {answer}")

This gives you confidence in your solution. Usually the example data is fairly trivial, so the time needed to solve the problem twice isn’t noticeably increased.

Use a __main__ guard

Now that we have a function, it is possible to import this “module” into another program to reuse the function. Except, it runs that pesky code at the bottom, generating unexpected output. Using a __main__ guard, the code will only execute when we run this script, not when this scripted is imported:

if __name__ == '__main__':
    assert sum_of_multiples_of_3_or_5_below(10) == 23

    answer = sum_of_multiples_of_3_or_5_below(1000)
    print(f"sum of all multiples of 3 or 5 below 1000 is {answer}") 

Generalization

This function is still perhaps too specific. Why just below a limit. Why just multiples of 3 or 5? We can generalize things a wee bit, and maybe actually increase the possibility of reusing the function elsewhere. And perhaps more importantly, explore the capabilities of Python.

First, instead of passing in the limit, let’s pass in the range.

def sum_of_multiples_of_3_or_5_in(iterable):
    total = 0
    for n in iterable:
        if n % 3 == 0 or n % 5 == 0:
            total += n
    return total

if __name__ == '__main__':
    assert sum_of_multiples_of_3_or_5_in(range(0, 10)) == 23

    answer = sum_of_multiples_of_3_or_5_in(range(0, 1000))
    print(f"sum of all multiples of 3 or 5 below 1000 is {answer}") 

range is a first class object in Python. It can be passed as an argument. So now you can easily compute the sum of the multiples of 3 or 5 in range(1000, 2000).

Or ... any iterable object, actually, such as lists.

print(sum_of_multiples_of_3_or_5_in([10, 12, 15, 17, 18, 19, 20])

How about those multiples? Let’s make them more general:

def sum_of_multiples_in(iterable, m1, m2):
    total = 0
    for n in iterable:
        if n % m1 == 0 or n % m2 == 0:
            total += n
    return total

if __name__ == '__main__':
    m1 = 3
    m2 = 5
    assert sum_of_multiples_in(range(0, 10), m1, m2) == 23

    answer = sum_of_multiples_in(range(0, 1000), m1, m2)
    print(f"sum of all multiples of {m1} or {m2} below 1000 is {answer}") 

You had a formula for computing the answer before. Sum of multiples of 3, plus sum of multiples of 5, minus sum of multiples of 15. Now it is harder, because m1 could be a multiple of m2 or vis versa. More cases to check for. But the above works just fine regardless of whether m1 and m2 are mutually prime or not.

any

Why only multiples of 2 numbers? Why not multiples of 3, 5 or 7? Passing yet another argument to the function seems wrong, because we’ll then need another function for 4 multiples, and yet another for 5 multiples. Let’s instead pass a list.

def sum_of_multiples_in(iterable, multiples):
    total = 0
    for n in iterable:
        for m in multiples:
            if n % m == 0:
                total += n
                break
    return total

That’s a good start. For each value of n, we start looping of the multiples, and if we find one, we add n to total and break out of the inner loop, to continue with the next n value.

But we can make it clearer. We want to know if n is a multiple of any of the multiples. Python has an any() function, which is true of any of the terms is true:

def sum_of_multiples_in(iterable, multiples):
    total = 0
    for n in iterable:
        if any(n % m == 0 for m in multiples)
            total += n
    return total

There is also an all(...) function which returns true only if all of the terms are true. Not needed here, but good to have in your back pocket.

sum

Now that we have a loop, an accumulator, and a filter condition, we can combine the three into a single sum() operation:

def sum_of_multiples_in(iterable, multiples):
    return sum(n for n in iterable if any(n % m == 0 for m in multiples))

Variable arguments

Using our above function, we have to pass in a list of multiples:

assert sum_of_multiples_in(range(0, 10), [3, 5]) == 23

It may be desirable to get rid of that explicit list [3, 5], and just pass in the arguments 3, 5 like we did earlier. We can do this by using a variable argument list syntax.

def sum_of_multiples_in(iterable, *multiples):
    return sum(n for n in iterable if any(n % m == 0 for m in multiples))

assert sum_of_multiples_in(range(0, 10), 3, 5) == 23

After all explicit arguments (iterable in this case), all remaining (non-keyword) arguments are rolled up into one list and assigned to the *args argument ... named multiples in this case.

"""Docstrings"""

Comments are used to describe the code to someone reading the source code. Doc-strings are used to describe how to use the code you’ve written, without the user needing to read your code. Various tools exist to extract the doc-strings, and turn them into webpages, PDF documents and so on. The simplest is Python’s built-in help() command.

"""
A collection of functions for solving problems from Project Euler.
(Currently, only Problem 1)
"""

def sum_of_multiples_in(iterable, *multiples):
    """
    From a list of numbers, return the sum of those numbers which
    are a multiple of one or more of the remaining arguments.
    """

    return sum(n for n in iterable if any(n % m == 0 for m in multiples))

if __name__ == '__main__':
    m1 = 3
    m2 = 5
    assert sum_of_multiples_in(range(0, 10), m1, m2) == 23

    answer = sum_of_multiples_in(range(0, 1000), m1, m2)
    print(f"sum of all multiples of {m1} or {m2} below 1000 is {answer}") 

A doc string is a string appearing at the top of a module, class, and/or function. It can be a single quoted string ("docstring" or 'docstring') or a triple quoted string ("""docstring""" or '''docstring'''). Triple quoted strings are typically used since they can span multiple lines and can contain quotes without needing escaping.

Save the file as pe1.py, then from a Python interpreter, type:

>>> import pe1
>>> help(pe1)

to see your help documentation.

Type Hints

Coming from C++, you will be used to a more “type safe” environment. Python’s fast and loose rules for type safety may be a wee bit difficult to get used too. Fortunately (or unfortunately), Python 3.6 and later allows you to specify “type hints”. These do absolutely nothing ... at least, as far as the Python interpreter is concerned. They can be read by static analysis tools, which can reason about them and ensure variables are being used in their intended fashion. If used for nothing else, they can provide additional “documentation” about the types of arguments for functions, and the return type of the function.

def sum_of_multiples_in(iterable: int, *multiples: int) -> int:
    ...

You can use type hints on local variables as well:

    total: int = 0

Hope this jump starts your exploration of Python. And once again, welcome to Python!

\$\endgroup\$
  • \$\begingroup\$ Thank you so much for investing the time to write such a detailed review! I learned quite a few new things. Some of this may be a bit advanced for me at this moment, so it's probably beneficial for me to reread this maybe a month later I guess :) \$\endgroup\$ – L. F. Sep 11 at 9:59
  • \$\begingroup\$ You’re welcome. Feel free to ask questions in a month. :-) \$\endgroup\$ – AJNeufeld Sep 12 at 0:03
5
\$\begingroup\$

The keyword sum is a built in function and you shouldn't name variables from within the reserved keywords.

Here's a list of the most common used keywords which you shouldn't be naming any of your variables:

[False, class, finally, is, return, None, continue, for, lambda, try, True, def, from, nonlocal, while, and, del, global, not, with, as, elif, if or, yield, assert, else, import, pass, break, except, in, raise]

And for the built-in functions check https://www.programiz.com/python-programming/methods/built-in

for n in range(0, 1000):

Can be written for n in range(1000):

The range() function as well as the whole Python is zero-indexed.

You might also want to use comprehension syntax (which is much more efficient than explicit loops) and enclose it inside a function like this:

def get_multiples(upper_bound):
    """Return sum of multiples of 3 and multiples of 5 within specified range."""
    return sum(number for number in range(upper_bound) if not number % 3 or not number % 5)
\$\endgroup\$
  • 1
    \$\begingroup\$ Thank you for the review! I'll definitely avoid the reserved identifiers in the future. \$\endgroup\$ – L. F. Sep 7 at 12:40
  • 1
    \$\begingroup\$ And by the way, predefined identifiers are different from keywords. \$\endgroup\$ – Roland Illig Sep 7 at 12:57
  • 4
    \$\begingroup\$ The function name get_multiples sounds as if it returned a sequence, but it doesn't. The function should better be called sum_of_multiples. \$\endgroup\$ – Roland Illig Sep 7 at 12:58
  • 2
    \$\begingroup\$ Using the not operator on numbers is confusing, at least for me. The code more clearly expresses its intention by saying number % 3 == 0. \$\endgroup\$ – Roland Illig Sep 7 at 13:02
  • 1
    \$\begingroup\$ sum is a built-in function but not a keyword. Bulit-ins can be overriden while assignments to keywords would result in syntax errors. \$\endgroup\$ – GZ0 Sep 7 at 17:24
4
\$\begingroup\$

The problem can be solved using math formulas, which will lead to an O(1) solution. But here I show another way to implement it, which is more efficient than a naive loop:

def sum_multiples(n):
    return sum(range(0, n, 3)) + sum(range(0, n, 5)) - sum(range(0, n, 3*5))

Or alternatively,

def sum_multiples(n):
    numbers = range(n)
    return sum(numbers[::3]) + sum(numbers[::5]) - sum(numbers[::3*5])

By the way, on the HackerRank website, the original problems are modified to require handling a wide range of inputs. You can run your solutions (with appropriate input / output added) on various provided test cases to verify the correctness under a time constraint.

\$\endgroup\$
  • \$\begingroup\$ Yeah, I am tempted to type in something like 3*(333*332/2) + 5*(200*199/2) - 15*(67*66/2) into my calculator and see the result ... \$\endgroup\$ – L. F. Sep 8 at 1:40
  • \$\begingroup\$ You could go to the website and try to implement that O(1) solution yourself :) You need that to pass all the test cases because the input can be up to 10^9 so a linear solution would result in timeouts. \$\endgroup\$ – GZ0 Sep 8 at 1:48

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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