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

Question

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.

• Roland Illig: 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: I'm especially thinking about codereview.stackexchange.com/a/280, which is really fast.
• AlexV: Is i % 35 really the condition you would like to check?

Answer 1

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.

• Bailey Parker: 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.
• Sigur: 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,...
• DjVasu: @Sigur yeah you can take two sets and apply union also.