[Tutor] Re: Request for code critique
Andrei
project5 at redrival.net
Wed Oct 22 13:59:41 EDT 2003
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.
More information about the Tutor
mailing list