Saving high scores in a pickle database

Question

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()

Answer 1

• 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.

Answer 2

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))

Answer 3

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.