Simple Scrabble Game Scorer

Question

I am making a program that scores a scrabble game and this is the basic version of it. Eventually I am going to add logic for double and triple words and letters but before I get too ahead of myself I would like to improve the basic code here.

All suggestions are welcome.

def main():
    letter_val = {" ": 0, "a": 1, "b": 3, "c": 3, "d": 2, "e": 1, "f": 4, "g": 2, "h": 4, "i": 1, "j": 8, "k": 5, "l": 1, "m": 3, "n": 1, "o": 1, "p": 3, "q": 10, "r": 1, "s": 1, "t": 1, "u": 1, "v": 4, "w": 4, "x": 8, "y": 4, "z": 10}
    players = []
    """ Function to add as many players as there are in the game (uses scrabble rules for limit) """
    def add_players():
        while True:
            while len(players) < 4:
                pla = input("Enter Player Names (can have up to 4) >>> ")
                if pla:
                    if pla not in (a for i in players for a in i):
                        if len(players) < 4:
                            players.append([pla, 0])
                            print("Player {} added".format(pla))
                        else:
                            return
                    else:
                        print("Name already in players' list")
                else:
                    return
            else:
                return

    def home(undo_ind=False):
        option = input('Would you like to [A]dd a score, [V]iew scores, [U]ndo the last change, or [End] the game? > ')

        class Score:
            def __init__(self):
                global temp_v, temp_p
                player = temp_p = input("Enter player to score>>> ")
                if player in (a for i in players for a in i):
                    try:
                        word = input("Enter word to score>>> ")
                        value = temp_v = sum(letter_val[i.lower()] for i in word)
                    except KeyError:
                        print("Word must consist of letters only.")
                        Score()
                    for i in players:
                        if i[0] == player:
                            i[1] += value
                else:
                    print("Player entered is not in player list.")
                home()

            @staticmethod
            def undo():
                try:
                    for i in players:
                        if i[0] == temp_p:
                            i[1] -= temp_v
                            home(True)
                except NameError:
                    print("No changes have been made.")
                    home()

            @staticmethod
            def view_scores():
                for i in players:
                    print("Player %s has a score of %d" % (i[0], i[1]))
                home(undo_ind)
        if option.lower() == "a":
            Score()
        elif option.lower() == "v":
            Score.view_scores()
        elif option.lower() == "u" and undo_ind is False:
            Score.undo()
        elif option.lower() == "u" and undo_ind is True:
            print("No changes have been made.")
            home(True)
        elif option.lower() == "end":
            print("Final scores are:")
            for i in players:
                print("Player {} has a final score of {}.".format(i[0], i[1]))
        else:
            print("That is not a valid option.")
            home(undo_ind)
    add_players()
    home()


if __name__ == "__main__":
    main()

Answer 1

I apologize that this answer became a "stream of consciousness"-type answer. I found it difficult to properly categorize what I wanted to say.

---

There's a fair amount to be said in add_players. The way you have things set up is quite confusing. You have fairly deep nesting, and multiple returns at multiple different levels of nesting.

if pla not in (a for i in players for a in i) is also confusing. Think about for i in players. You're referring to each player as i? Also, your intent with that generator expression seems to be to flatten players so you can check if a name already exists. Flattening doesn't seem necessary though since each "player" is a name and score. Why check if a name is equal to a score?

You also have a while len(players) < 4 inside of a while True. I can't really see the point of the while True though.

I would also use much more descriptive names. Once you reduce the nesting, you'll have a lot more room to work with.

Finally, add_players really shouldn't be adding to a global players. players should be returned from add_players and assigned at the call site.

Altogether, I'd write this closer too:

def add_players():
    players = []

    while len(players) < 4:
        new_name = input("Enter Player Names (can have up to 4) >>> ")

        if new_name:
            if new_name not in (name for name, _ in players):
                players.append([new_name, 0])
                print("Player {} added".format(new_name))

            else:
                print("Name already in players' list")

        else:
            break

    return players

Notice now there's a single return, and it's returning the players for the caller to use.

---

Please don't take this the wrong way, but the design of the rest of the program doesn't make much sense:

• Why is everything inside of main? Ideally, the main function should be a small function at the end of your program that just calls a few other functions.

• Why is a Score class inside of the home function?

• Score also doesn't seem like it should even be a class. All of the methods of the class are static, and it seems like you're only using Score to score a word by calling the Score constructor. If you never use an instance of a class, it shouldn't be a class. Just make Score into a score_word function. I'd also pass a word into the function instead of asking inside of the function. Functions are harder to test when they produce their own data.

• You're catching a NameError in undo. I'm guessing this is in case Score() hadn't been called yet? Don't do this. I can't think of a time when it's ever appropriate to catch a NameError. Any time a NameError happens, it means you have flawed logic in your program. You should fix the issue instead of putting an try/except band-aid over it. If the function requires temp_p and temp_v (which need better names), you need to ensure that that data is available. Either pass it in, or ensure that they're properly initialized ahead of time.

---

---

There's more that can be dug out here, but I need to start getting ready for work. I'll just make a few broad suggestions:

• Please take much more care when writing code. A lot of this code seems like it was written quickly without much second-thought. I would focus on the intent of the code before it's written.

  Ask yourself, "What exactly should this code do?", and "What are the appropriate tools to do what I'm trying to do?". Doing things like using classes as normal functions and writing code like (a for i in players for a in i) that technically works (but doesn't really do exactly what you want) makes your code hard to understand, and will make it difficult to add to later.

• Don't nest everything. Really, all of these functions should be "top-level", outside of everything else. If you nest thing A inside of thing B, that suggests that A is deeply connected to B and wouldn't have a valid meaning outside of B. Really though, that's not the case here. Score would have the same meaning even if it were outside of home, and all the functions would have the same meaning even if they were outside of main.

  Nesting conveys a certain association meaning, and if used improperly, gives the reader the wrong initial idea about how the code works. Nesting also forces you to use more indentation, which generally makes code harder to read.

With the above criticisms out of the way, I'll note what is good here:

• You're following proper naming guidelines. You have functions starting with lowercase, classes starting with uppercase, and _ to separate "words" in names.

• You're using idiomatic shortcuts like if pla: to check if a collection is empty or not.

• You tried to have all the functionality start from a main instead of being loose in the script. This makes code easier to run and test.

• letter_val is a good use of a dictionary.

  • I'll note though that it's a constant, so it should be in all uppercase.

Keep at it. There's a lot to be improved here, but there's promise too.