Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

Seeking to improve upon my high score 'module' code. It saves users' high score in a pickle database and if needed prints out the current scores in the database.

import pickle
import os
global high_scores

def setup_scores():
    global high_scores
    high_scores = {}

    if os.path.isfile('highscores.pkl'):
        with open("highscores.pkl", "rb") as h:
            high_scores = pickle.load(h)
    else:
        high_scores = {"Adam Smith": 65536, "John Doe": 10000}

def save_score(name, score):
    new_score = (name, score)

    if new_score[0] in high_scores:
        if new_score[1] > high_scores[new_score[0]]:
            high_scores[new_score[0]] = new_score[1]
    else:
        high_scores[new_score[0]] = new_score[1]

    with open("highscores.pkl","wb") as out:
        pickle.dump(high_scores, out)

def print_scores():
    for name, score in high_scores.items():
        print("{{name:>{col_width}}} | {{score:<{col_width}}}".format(col_width=(80-3)//2).format(name=name, score=score))

setup_scores()
save_score(raw_input('Name:'), raw_input('Score:')) # inputs only for testing, can use variables instead
print_scores()
share|improve this question

3 Answers 3

up vote 11 down vote accepted
  • Use of global should generally be avoided. You could create a class to hold the dictionary and the three functions would become methods.
  • Don't repeat yourself: use a variable for the file name so you don't have to change it in three places if it needs to change.
  • The save_score function assigns new_score = (name, score) for no reason at all. Using the two variables directly in the code that follows would improve readability.
share|improve this answer

Building on Janne Karila's review:

global high_scores

Personally I find your use of a global variable fine. Using a singleton class for encapsulation where a module would do is IMO needless complexity. If you can have multiple high score objects it's a different matter, of course.

with open("highscores.pkl","wb") as out:
    pickle.dump(high_scores, out)

By default pickle uses an ASCII format, so you shouldn't really open the file in binary mode, although nothing should break if no one touches the file. You could leave out the bs from both open calls or use binary pickling by passing protocol=-1 to pickle.dump.

    print("{{name:>{col_width}}} | {{score:<{col_width}}}".format(col_width=(80-3)//2).format(name=name, score=score))

You could leave out that second col_width – it will cause an extra unnecessary line break if the name overflows the limit and will not alter normal output in any visible way.

Calculating the column width is also independent of the current score, so you could move it out of the loop, breaking the overlong line:

score_line = "{{name:>{col_width}}} | {{score}}".format(col_width=(80-3)//2)
for name, score in high_scores.items():
    print(score_line.format(name=name, score=score))
share|improve this answer

I have no opinion about the global issue. I have a weak preference for encapsulation, but I don't think it's a big deal.

However, both commenters so far missed the fact that save_scores shouldn't write the file if no changes are made; if the user already has a score registered which is higher than the current score, don't write the file.

Also, a tiny nitpick: your test doesn't coerce the score into an int.

share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.