8
\$\begingroup\$

I saw the Project Euler challenges online and I decided to code out a few. The first challenge involved finding the sum of multiples. So I decided to have my code be able to find the sum of multiples for any given range, and for any amount of natural numbers.

My code allows a user to input as many natural numbers as they want and then set the range of multiples that user wants to sum and then sum those multiples.

I just wanted any input on if there's a more efficient way of doing this.

def main():
    # Initialize variables
    multiples = []
    sums = []
    i = 0
    integers = 1

    # Detemines the number of multiples from user
    while True:
        try:
            num_mul = int ( input ( 'How many multiples are you using in total?: ' ) )
            break
        except ValueError as e:
            print ( e )

    # Takes multples and store in list
    while True:
        try:
            for y in range ( num_mul ):
                multiples.append ( int ( input ( 'Please input your No.{} integer here: '.format(integers) ) ) )
                integers = integers + 1
            break
        except ValueError as e:
            print ( 'Integer Values ONLY' , e )

    # determine the start and stop of the range from user
    while True:
        try:
            start = int ( input ( 'Start of range: ' ) )
            stop = int ( input ( 'End of range: ' ) ) + 1
            break
        except ValueError as e:
            print ( e )

    # Figures out the multiples of the numbers given between the start and end range
    while i < len ( multiples ):

        for k in range (start, stop ):
            if (k % multiples[i]) == 0:
                sums.append(k)
            else:
                continue

        i = i + 1  # increments through the list values by 1

    x = sum(set(sums))  # Gets rid of duplicated values and sums the list of values

    print ( 'Your SUM total is {}.'.format ( x ) )


if __name__ == "__main__": main ( )
\$\endgroup\$
7
\$\begingroup\$

there is some room for improvement here, I tried to adapt a bit your solution without changing it a lot, to make it a bit less repetitive and more understandable

Will explain you some steps :)

Reading input

First of all, you are reading the input from the user on a few places, always in the same way, asking for input again unless an integer is provided. To avoid repetition, this functionality can be extracted into a function

def read_integer(input_message):
    while True:
        try:
            number = int(input(input_message))
            return number
        except ValueError:
            print('Integer Values ONLY')

You just pass the message you want to appear on screen and it will ignore all non integer values, asking for input again

Reuse of variables

Sometimes, you can use same variable for different purposes.

In order to read input in a loop, you can use the same variable that is introduced in the loop as an index, avoiding the creation of the new variable integers only for that

So this code

integers = 1
for y in range ( num_mul ):
    multiples.append ( int ( input ( 'Please input your No.{} integer here: '.format(integers) ) ) )
    integers = integers + 1

Can be reduced to

for y in range ( num_mul ):
    multiples.append ( int ( input ( 'Please input your No.{} integer here: '.format(y+1) ) ) )

Avoid unnecessary code

In the end, there is an else condition that simply continues to next iteration. That will also happen even if the else condition does not exist, saving you some unnecessary lines of code

A bit later, you turn all multiples into a set to avoid repetition. That can be handled before, inserting only if they are not already in the list

if (k % multiples[i]) == 0 and k not in sums:
    sums.append(k)

Naming

You could use a bit better naming in variables and so, I modified a bit some of them so you can check but things like num_mul can be better understood with longer naming, like multiples_count

Summary

You can check how your solution would look like with this features, I applied the changes that I suggested you:

def read_integer(input_message):
    while True:
        try:
            number = int(input(input_message))
            return number
        except ValueError:
            print('Integer Values ONLY')


def main():
    # Initialize variables
    multiples = []
    sums = []

    # Determines the number of multiples from user
    input_message = 'How many multiples are you using in total?: '
    multiples_count = read_integer(input_message)

    # Takes multiples and store in list
    for y in range(multiples_count):
        input_message = 'Please input your No.{} integer here: '.format(y+1)
        new_multiple = read_integer(input_message)

        multiples.append(new_multiple)

    # determine the start and stop of the range from user
    input_message = 'Start of range: '
    start = read_integer(input_message)

    input_message = 'End of range: '
    stop = read_integer(input_message)

    # Figures out the multiples of the numbers given between the start and end range
    for i in range(multiples_count):
        for k in range(start, stop):
            if (k % multiples[i]) == 0 and k not in sums:
                sums.append(k)

    x = sum(sums)  # Gets rid of duplicated values and sums the list of values

    print('Your SUM total is {}.'.format(x))


if __name__ == "__main__":
    main()
\$\endgroup\$
  • 5
    \$\begingroup\$ I would eliminate input_message as a useless variable. \$\endgroup\$ – 200_success Mar 4 '18 at 5:07
  • 3
    \$\begingroup\$ And I would make sums a set directly. \$\endgroup\$ – Graipher Mar 4 '18 at 8:56
  • \$\begingroup\$ Thanks for this. Will check over my code and move forward with these ideas. \$\endgroup\$ – The ONLY One Mar 4 '18 at 13:31
  • \$\begingroup\$ @A. Romeu I am wondering why you didn't use the +1 with stop since range is non-inclusive? If we are asking the user to enter the "End of Range" value. \$\endgroup\$ – The ONLY One Mar 4 '18 at 14:19
  • 1
    \$\begingroup\$ I didn't modify that loop in particular, it's exactly as you stated on the description on your question \$\endgroup\$ – A. Romeu Mar 4 '18 at 15:03
4
\$\begingroup\$

A more beautiful loop

Starting from this piece of code, we'll start going for a more elegant implementation:

i = 0
...
while i < len ( multiples ):

    for k in range (start, stop ):
        if (k % multiples[i]) == 0:
            sums.append(k)
        else:
            continue

    i = i + 1  # increments through the list values by 1

We could start by shaving off the un-necessary: the blank lines and the else-continue.

We can also bring the i = 0 part closer to the block actually using it.

Also, the comment # increments through the list values by 1 does not bring much information. Anytime you add a comment, you can ask yourself "could this actually help anyone reading the code ?". A common rule of thumb is to document the "why" rather than the "how": tell what you want/need to do and why rather than how you do it.

Finally, the i = i + 1 can be written i += 1.

At this stage, we have:

i = 0
while i < len ( multiples ):
    for k in range (start, stop ):
        if (k % multiples[i]) == 0:
            sums.append(k)
    i += 1

Then, things can still be improved. For a start, you call the len function at each iteration. It's not a very expensive operation but it is still un-necessary. However, the actual part to change is to switch the while loop for a for loop: your situation looks like the typical situation to use a for loop. Using the range function you already know:

for i in range(len(multiples)):
    for k in range (start, stop ):
        if (k % multiples[i]) == 0:
            sums.append(k)

Please note that it removed the need to initialise i, to increment it after each iteration and the len function is only called once. This looks like the perfect solution, doesn't it ?

Not quite! This loop uses an index to go through elements of a container. This is how most people (used to) do in various programming languages. However Python defines objects that are iterable and a clean syntax to use them. You can simply write for num in multiples. This is very well explained by Ned Batchelder's talk "loop like a native"

for mul in multiples:
    for k in range(start, stop):
        if (k % mul) == 0:
            sums.append(k)

Also, this could be written using a list comprehension, but I'd rather avoid making things too complicated for now.

Handling duplicated values

You have different ways to handle duplicated values with pros and cons. At the moment, you put all values in a list then convert it to a set. A. Romeu's answer suggest checking if the value is in the container before adding it (which can be inefficient because then, you'd need to iterate over the whole list before adding a new value to know it is not there yet).

Graipher's comment suggested using a set directly. This is a pretty good solution. You'd have something like:

sums = set()
for mul in multiples:
    for k in range(start, stop):
        if (k % mul) == 0:
            sums.add(k)

Not much to change :-)

For the sake of learning new things, we could go for a different approach. What if the best way to handle duplicates was not to have duplicates in the first place. At the moment, your code iterate over range(start, stop) many times. Changing the code a bit, we could iterate over it only once.

sums = []
for k in range(start, stop):
    # TODO: if a number in multiples divides k
        sums.append(k)

This could be:

sums = []
for k in range(start, stop):
    for mul in multiples:
        if k % mul == 0:
            sums.append(k)
            break

Making your code testable

The Project Euler problem gives you an example of input and expected output. You could take this chance to write tests to ensure that your code is valid regarding this example. Also, this would lead to writing smaller, easier to reuse and to understand function. We could imagine writing something like: def sum_of_multiples(limit, multiples) and checking that sum_of_multiples(10, [3, 5]) == 23.

Mathematical optimisation

It doesn't really matter on this problem but the further you go into Project Euler problems, the less it becomes about writing a straight-forward bruteforce solution and the more it becomes about finding mathematical properties to be able to write an efficient solution.

On this problem, there is already a much more satisfying solution to mathematicians. The key insight is that we have formulas to compute the num of number from i to j. This can easily be used to compute the sum of multiples of 3 or 5 between i and j. Then, it leads to the number you're looking for (note: multiples of 15 are counted twice but you can once again compute their sum easily).

\$\endgroup\$
  • \$\begingroup\$ This is well explained and encouraging. Thank you! \$\endgroup\$ – The ONLY One Mar 4 '18 at 13:29
  • \$\begingroup\$ Good insight on using a set for the sake of efficiency, nicely pointed ;) \$\endgroup\$ – A. Romeu Mar 4 '18 at 15:05

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.