# Project Euler #1: Sum of Multiples of 3 and 5 below 1000

def prob_1():
sum_mult=[]  #Create an empty list which will take sum of multiples of 3 and 5
check_sum=0
for i in range(1,1000): #Take numbers till 1000
#if(i)
if( (i%3)==0 or (i%5)==0 ): #divisor condition
sum_mult.append(i)

return sum(sum_mult)    #return sum of list


I am just starting out my journey as a programmer, here is my code and I would love to see any critical feedback and other alternative solutions maybe using some clever hack of using lambda function's etc.

• To make this program really fast, look at any other review on this site that is also about Project Euler #1. The programming language doesn't matter, it's basically the same in all languages. – Roland Illig Apr 17 at 6:05
• I'm especially thinking about codereview.stackexchange.com/a/280, which is really fast. – Roland Illig Apr 17 at 6:18
• Is i % 35 really the condition you would like to check? – AlexV Apr 17 at 7:00

I expect you made a typo. You don't want (i%35)==0, you want (i%5)==0.

The PEP-8 style guide for Python requires 1 space before and after operators, and after commas. Use PyLint or equivalent tool to ensure you follow all of the PEP-8 guidelines.

check_sum is unused, and can be omitted.

The brackets around the if( ... ): condition are unnecessary. This is Python, not C, C++ or Java:

    if (i % 3) == 0 or (i % 5) == 0: #divisor condition


There is no need to create a list just to add up all the numbers after the fact. You are only using each value once, so you could simply add the numbers up as you find them:

def prob_1():

sum_of_multiples = 0

for i in range(1, 1000):  # Take numbers up to but not including 1000
if (i % 3) == 0 or (i % 5) == 0: #divisor condition
sum_of_multiples += i

return sum_of_multiples


You should add """doc_strings""" to your functions:

def prob_1():
"""
Compute the sum of all the multiples of 3 or 5 below 1000.

Returns:
The sum of the multiples of 3 or 5, below 1000.
"""

sum_of_multiples = 0

for i in range(1, 1000):  # Take numbers up to but not including 1000
if (i % 3) == 0 or (i % 5) == 0: #divisor condition
sum_of_multiples += i

return sum_of_multiples


You can use list comprehension a generator expression (thanks @Graipher) and the sum(...) function to compute the result, without ever creating the list in memory:

def prob_1():
"""
Compute the sum of all the multiples of 3 or 5 below 1000.

Returns:
The sum of the multiples of 3 or 5, below 1000.
"""

return sum(i for i in range(1000) if i % 3 == 0 or i % 5 == 0)


You can also solve this problem by hand with a pen, a sheet of paper, a calculator and about 1 minute of your time. A program is entirely unnecessary.

• You eventually arrive at it, but I'd also add that it is not common to parenthesize the modulo operator (or most other infix operators) like that. i % 3 == 0 is preferred to (i % 3) == 0. – Bailey Parker Apr 17 at 21:00
• Should be and instead of or, I think. OP wants multiple of 3 and 5. So, 3,6,9,12,15,... intersects 5,10,15,20,... – Sigur Apr 17 at 23:21
• @Sigur yeah you can take two sets and apply union also. – DjVasu Apr 18 at 6:17