2

I am trying to get the sieve of Eratosthenes here but the code keeps returning the original list. I have no idea what is wrong. Please do help a poor novice- and thank you very much in advance!!

def main():
    cupcake = []
    i = 0
    for i in range(1, 101):
        cupcake += [i]
    cupcake.remove(1)
    cupcake.insert(0, 0)
    for j in range(len(cupcake)):
        if cupcake[j] > 0:
            for k in range(len(cupcake)):
                if cupcake[k] > 0:
                    product = int(cupcake[k]) / int(cupcake[j])
                    if (type(product) is int) == True:
                        if product == 1:
                            continue
                        cupcake.remove(cupcake[k])
                        cupcake.insert((k-1), 0)
    print(cupcake)
B D
  • 29
  • 2

3 Answers3

2
product = int(cupcake[k]) / int(cupcake[j])

In this statement product will always be a float value, even if it is a whole number (2.0, ...). So the statement after it will always return false. You have to change the statement after it from 

if (type(product) is int) == True:

to

if product.is_integer():

which uses the float.is_integer() functions.

Robin
  • 303
  • 1
  • 4
  • 16
1

That is awfully inefficient:

  • you build the initial list one element at a time when a list comprehension would do it in one single operation: cupcake = i for i in range(1, 101) or even cupcake = list(range(1, 101))
  • you use the anti-pattern remove + insert to change the value of an element of the list: cupcake[0] = 0 and later in the code cupcake[k] = 0
  • you loop from the begining for j and k, when the second loop could start at j+1
  • you (wrongly) use division when you should use modulo

And your real problem is that in Python 3, the division of two integers is always a float whether one is or is not a multiple of the other:

>>> 4 / 2
2.0
>>> type(4/2)
float

So your code could become:

def main():
    cupcake = list(range(1, 101))
    cupcake[0] = 0
    for j in range(len(cupcake)):
        if cupcake[j] > 0:
            for k in range(j+1, len(cupcake)):
                if cupcake[k] > 0:
                    if cupcake[k] % cupcake[j] == 0:
                        cupcake[k] = 0
    print(cupcake)

Anyway, the correct algo is to remove multiples instead of consistently testing divisibility:

def main():
    cupcake = list(range(1, 101))
    cupcake[0] = 0
    for j in range(len(cupcake)):
        if cupcake[j] > 0:
            for k in range(j + cupcake[j], len(cupcake), cupcake[j]):
                        cupcake[k] = 0
    print(cupcake)
Serge Ballesta
  • 143,923
  • 11
  • 122
  • 252
  • Hi Serge- thanks for you answer. Wow- I didn't quite expect there to be such a short way. I'm making my way through my first-ever coding exercises- please do bear with me. Hope to be as good as you sometime ^^ – B D Dec 18 '17 at 11:29
0

I wouldn't write it like that but here goes my improvement suggestion for your code:

def main():
     cupcake = [i for i in range(101)]
     cupcake.remove(0)
     cupcake.remove(1)
     for j in cupcake:
         if j > 0:
             for k in cupcake:
                 if k > j and k % j == 0:
                     cupcake.remove(k)
     print(cupcake)

main()
Veltzer Doron
  • 934
  • 2
  • 10
  • 31