3
\$\begingroup\$

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()
\$\endgroup\$
3
  • 3
    \$\begingroup\$ Consider asking 5 separate questions. \$\endgroup\$
    – vnp
    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
    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\$ Dec 6, 2017 at 13:14

2 Answers 2

1
\$\begingroup\$

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.

\$\endgroup\$
5
\$\begingroup\$

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.

\$\endgroup\$
0

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

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