To continue learning I decided to try solving some of project Euler problems. Here is the first solution.
The things I'm interested the most from the review are:
- The performance of the code
- Overall review of the code structure, styling rules and naming conventions.
Problem: 1
Find the sum of all the multiples of 3 or 5 below 1000.
#----------------------------------------------------------------------------------
def calculate_the_sum(threshold = 10):
"""Calculates the sum of all multiples of 3 or 5
below the provided threshold.
Arguments:
type:<int> -Preforms the calculations on all values up to this value.
Return:
type:<int> -The sum of numbers that are multiples of 3 or 5..
"""
return sum(n for n in range(threshold) if n % 3 == 0 or n % 5 == 0)
#----------------------------------------------------------------------------------
def main():
threshold = 1000
print(' Calculates the sum of all numbers that are multiples of 3 or 5')
print(' The Sum of numbers below {} is: {}'.format(threshold,calculate_the_sum(threshold)))
#----------------------------------------------------------------------------------
if __name__ == "__main__":
main()
-
3\$\begingroup\$ Consider asking 5 separate questions. \$\endgroup\$vnp– vnp2017年12月06日 06:06:27 +00:00Commented Dec 6, 2017 at 6:06
-
\$\begingroup\$ @vnp Should i split this post, or you meant that for the future posts i make ? \$\endgroup\$HelloWorld– HelloWorld2017年12月06日 12:11:49 +00:00Commented Dec 6, 2017 at 12:11
-
\$\begingroup\$ There is too little in common between the five problems; we want to evaluate your solution, not you as a programmer. Since there is currently one answer that addresses just Problem 1, I have removed 2–5. \$\endgroup\$200_success– 200_success2017年12月06日 13:14:47 +00:00Commented Dec 6, 2017 at 13:14
2 Answers 2
You can define a generic function to relate the sum for a given multiplier to the number of multiples below the threshold. This is shown in function calculate_sum3
.
import time
def ntimerprinter(n):
"""Decorate the function to run it n times and print the time
"""
def inner(func):
def wrapper(*args, **kwargs):
before = time.time()
for _ in range(n):
result = func(*args, **kwargs)
after = time.time()
elapsed = after - before
print 'Run {0.__name__} {1} times in: {2} s'.format(func, n,
elapsed)
return result
return wrapper
return inner
@ntimerprinter(500)
def calculate_sum1(threshold=10):
"""Calculates the sum of all multiples of 3 or 5
below the provided threshold.
Arguments:
type:<int> -Preforms the calculations on all values up to this value.
Return:
type:<int> -The sum of numbers that are multiples of 3 or 5..
"""
return sum(n for n in range(threshold) if n % 3 == 0 or n % 5 == 0)
@ntimerprinter(500)
def calculate_sum2(threshold=10):
"""Calculates the sum of all multiples of 3 or 5 below threshold.
"""
return (sum(range(3, threshold, 3)) + sum(range(5, threshold, 5)) - sum(
range(15, threshold, 15)))
@ntimerprinter(500)
def calculate_sum3(threshold=10):
"""Calculates the sum of all 3 and 5 entire multiples below threshold
"""
# number of multiples below the threshold
n3 = int((threshold - 1) / 3)
n5 = int((threshold - 1) / 5)
n15 = int((threshold - 1) / 15)
# generic sum function for multipliers m and highest multiple n
msum = lambda m, n: m * (n + (n * (n - 1)) / 2)
# respective sums
sum3 = msum(3, n3)
sum5 = msum(5, n5)
sum15 = msum(15, n15)
return sum3 + sum5 - sum15
if __name__ == '__main__':
t = 100000
print calculate_sum1(t)
print calculate_sum2(t)
print calculate_sum3(t)
Results:
Run calculate_sum1 500 times in: 4.82500004768 s
2333316668
Run calculate_sum2 500 times in: 0.351000070572 s
2333316668
Run calculate_sum3 500 times in: 0.000999927520752 s
2333316668
Regarding docstring and naming, a few remarks:
First, all of these are important if you intend to share your code publicly. Anyway, you would be glad to follow conventions if this day comes later.
Accroding to PEP8 naming convention:
Function names should be lowercase, with words separated by underscores as necessary to improve readability.
Basically, you followed it but the the
in calculate_the_sum
is certainly useless. Also, your name lacks of meaning as it do not refer to a sum of multiples. More generally, people tends to give meaningful short name without underscore to public function while longer fully understandable name for private ones. But no rules here, what matters the most is readability and consistency in the way you name your function.
According to PEP257 Docstring Conventions:
Multi-line docstrings consist of a summary line just like a one-line docstring, followed by a blank line, followed by a more elaborate description.
So, it would be better if your summary fit in one line (<120 characters as should be any line in your code, <80 for purist). They are several docstring conventions and the most important once you picked one is to stick to it.
Finally, have a look at the zen of python (PEP20). These are illustrated in The hitchhiker's guide to python (with more detail in the paper book). Specifically, one important aphorism is "sparse is better than dense". Basically, make one statement per line, add inline comments and separate them form the code using blank lines and in your case, do not return a long formula. Split it into several components and return something that appears meaningful. There is no pride in having the shortest code. Python is all about the opposite. Actually, important library that are following this principle are easy to read (for instance: pandas.Series) and it is such a pleasure to read it like English, learning from it, reviewing it and collaborate.
For the first problem I suggest you some performance improvements. You can use inclusion-exclusion technique for this problem.
So first improvement
def calculate_the_sum(threshold = 10):
return(sum(range(3, threshold, 3)) + sum(range(5, threshold, 5)) - sum(range(15, threshold, 15)))
def main():
threshold = 1000000
print(' Calculates the sum of all numbers that are multiples of 3 or 5')
print(' The Sum of numbers below {} is: {}'.format(threshold,calculate_the_sum(threshold)))
if __name__ == "__main__":
main()
I have tested both solutions in ideone (Python 3) for threshold=1000000. Your solution ~ 0.20s. My ~ 0.05s.
Next – you can use well known formula for the sum of arithmetic progression.
Explore related questions
See similar questions with these tags.