Struggling with basics

[Tom Anderson]

On Sun, 25 Sep 2005, Jason wrote:

> A week ago I posted a simple little hi-score routine that I was using to 
> learn Python.
>
> I've only just managed to examine the code, and the responses that people 
> gave, and I'm now seriously struggling to understand why things aren't 
> working correctly.

Others have dealt with the string printing problem, so i'll leave that.

The problem with the sorting is that you're not consistent about how 
scores are represented - are they strings or integers? At present, you 
sometimes use one and sometimes the other, with the result that the sort 
basically pukes all over you. To fix this, pick one type (hint: integers), 
and use that consistently. I'll show you how to do that below (although 
it's not exactly hard).

Oh, and i'm a picky git, so i'm going to point out some other flaws in the 
code!

> At present my code is as follows...
>
> import random
> import bisect
>
> class HiScores:
>    def __init__(self,hiScores):
>        self.hiScores=[entry for entry in hiScores]

One bug and one wart here.

The wart is the way you initialise self.hiScores - you use a list 
comprehension when you can just call the list builtin:

self.hiScores = list(hiScores)

The bug is that you don't sort the list. If you're certain that the 
initial set of high scores will always come sorted, that's okay, but i'd 
say it was good practice to sort them, just in case.

In fact, i'd punt the addition to addScore:

def __init__(self, hiScores):
 	self.hiScores = []
 	for score, name in hiScores:
 		self.addScore(score, name)

This is the 'Once And Only Once' principle in action; the knowledge about 
how to keep the list sorted is expressed once and only once, in addScore; 
if any other parts of the code need to add items, they call that. This 
means there's only one piece of code you have to check to make sure it's 
going to get this right.

>    def showScores(self):
>        for score,name in self.hiScores:
>            score=str(score).zfill(5)
>            print "%s - %s" % name,score

As has been pointed out, you need to wrap parens round "name, score" to 
make it into a tuple.

Apart from that, i'd skip the string interpolation and just write:

for score, name in self.hiScores:
 	print name, "-", str(score).zfill(5)

If you insist on the string interpolation, i'd still elide the 
intermediate variable and write:

for score, name in self.hiScores:
 	print "%s - %05i" % (name, score)

The %05i in the format string means 'an integer, zero-filled to five 
digits'. Good, eh?

>    def addScore(self,score,name):
>        score.zfill(5)
>        bisect.insort(self.hiScores,(score,name))
>        if len(self.hiScores)==6:
>            self.hiScores.pop()

Two problems there. Well, two and a half.

Firstly, the type confusion - are scores strings or integers? the zfill 
indicates that you're thinking in terms of strings here. You should be 
using integers, so you can just drop that line.

And if you were working with strings, the zfill would still be wrong (this 
is the half problem!) - zfill doesn't affect the string it's called on 
(strings are immutable), it makes a new zero-filled string and returns it. 
You're not doing anything with the return value of that call, so the 
zero-filled string would just evaporate into thin air.

Secondly, bisect.insort sorts the list so that the highest scores are at 
the tail end of the list; list.pop takes things off that same end, so 
you're popping the highest scores, not the lowest! You need to say pop(0) 
to specify that the item should be popped off the head (ie the low end) of 
the list.

Also, i'd be tempted to program defensively and change the test guarding 
the pop to "while (len(self.hiScores > 6):".

All in all, that makes my version:

def addScore(self, score, name):
 	bisect.insort(self.hiScores, (int(score), name))
 	while(len(self.hiScores) > 6):
 		self.hiScores.pop(0)

>    def lastScore(self):
>        return self.hiScores[-1][0]

This will return the top score; you want self.hiScores[0][0].

> def main():
>
>      hiScores=[('10000','Alpha'),('07500','Beta'),('05000','Gamma'),('02500','Delta'),('00000','Epsilon')]

Here you've got scores as strings, and this is the root of the problem. 
Change this to:

hiScores=[(10000,'Alpha'),(7500,'Beta'),(5000,'Gamma'),(2500,'Delta'),(0,'Epsilon')]

Note that i've taken the leading zeroes off - leading zeroes on integers 
in python are a magic signal that the number is octal (yes, base eight!), 
which is not what you want at all.

>    a=HiScores(hiScores)
>    print "Original Scores\n---------------"
>    a.showScores()
>
>    while 1:

"while True:" is preferred here.

>        newScore=str(random.randint(0,10000))

Take out the str().

>        if newScore  > a.lastScore():
>            print "Congratulations, you scored %s " % newScore

Make that a %i (or a %05i).

>            name=raw_input("Please enter your name :")
>            a.addScore(newScore,name)
>            a.showScores()
>
> if __name__=="__main__":
>    main()

Works like a charm!

> The final part that's simply not working correctly is that the entire program 
> isn't sorting the data.
>
> If I run the program and get a score of, say, 6789, then when I add my name, 
> nothing is entered.  I have changed the clause that deletes (pops) the last 
> array if the array count is 6 and seen what figures are being entered into 
> the array.
>
> Sure enough they are going in the array, and they are being sorted, but they 
> are only being sorted AFTER the 00000 of the initial array creation.
>
> I'm pretty sure it's to do with comparing a string against an integer 
> but can't for the life of me see where to force the comparrison to check 
> against two integers.

Simple - you just make sure the scores are all integers in the first 
place!

tom

-- 
double mashed, future mashed, millennium mashed; man it was mashed