[Tutor] Re: Request for code critique

[Andrei]

Matt Hehman wrote:

> Well, here goes.  Some of the comments wrapped, as did
> two of the longer lines, which I stuck in parentheses,
> so this should be about right.  Besides anything
> really glaring, what would I need to do if this was
> going to be a mere feature of a much longer program?  

For one thing, wrap everything up in classes or at the very least functions. If 
you import your module in a larger program, everything is executed at import 
time so the larger program can't really use it. Not to mention that if you 
program without classes (and even more so when you program without functions), 
the results are unmaintainable.

<snip>
> ##new primes are calculated if current list is
> ##insufficient
> if primes[-1]+1<ceiling:
>     candidates=range((primes[-1]+2),ceiling+1,2)

For the sake of clarity, I think it would be better if you added 1 to ceiling 
the first time you defined it.

>     n=1
>     result=[]

Why do you need results? You could just as well work directly on your primes 
list. Btw, I prefer spaces to the left and right of equal signs and to the richt 
of commas; makes code easier to read.

>     while candidates:
>         limit=int(candidates[0]**0.5)
>         while primes[n]<=limit and len(candidates)>0:
>             limit=int(candidates[0]**0.5)
>             if candidates[0]%primes[n]==0:
>                 del candidates[0]
>                 n=0
>             n=n+1
>         if len(candidates)>0:
>             result.append(candidates[0])
>             del candidates[0]
>             n=1

You should look into the pop() method which deletes an item and returns it as 
well. Also I think that your while-solution is not as pythonic as it could be 
(making it harder to follow for me). It's more common to use a break statement 
than to define a loop variable outside the while-loop:

# no limit variable outside the while loop
while len(candidates)>0:
     limit = int(candidates[0] ** 0.5)
     if primes[n]>limit: # stops the loop
         break
     # the rest of your code

Regardless of this, I'm not sure that deleting items is the fastest possible way 
to do it. In fact, I'm tempted to think that looping through the items in 
candidates would be faster (and IMO looks better too), using a for-loop:

for candidate in candidates:
     # find out if the candidate is indeed a prime
     limit = int(candidate ** 0.5)
     for prime in primes:
         if prime > limit:
             # we've found a new prime, go to next one
             primes.append(candidate)
             break
         elif candidate % prime == 0:
             # candidate is not a prime, go to next one
             break

This does assume that you start out with primes containing at least [2, 3] and 
is 50% shorter than your solution (not counting comments). Bonus points if you 
can get that in a list comprehension :).

> ##progressively slower with larger numbers - next
> ##planned revision
> factors=[]
> while primes and primes[0]<ceiling+1:
>     if Euclid(bignum,primes[0])>1:
>         factors.append(Euclid(bignum,primes[0]))
>     del primes[0]

Again, I think a for-loop might be more appropriate here, looping "for prime in 
primes".

<snip>

-- 
Yours,

Andrei

=====
Mail address in header catches spam. Real contact info (decode with rot13):
cebwrpg5 at bcrenznvy.pbz. Fcnz-serr! Cyrnfr qb abg hfr va choyvp cbfgf. V ernq gur 
yvfg, fb gurer'f ab arrq gb PP.