14
\$\begingroup\$

I am a beginner programmer and have recently finished coding a clone of the game Wordle.

I want to know if the way I programmed this game is efficient, solid, and overall good code. I also want to know how I can know whether my code in the future is good or not.

Here is my code:

(by the way, the opened txt file is a text file with almost 6000 5-letter words from the English dictionary I found online)

import random

lines = open("5_letter_words.txt", "r").read().splitlines()
word = random.choice(lines)
word = word.upper()


board = {
    6 : ["_", "_", "_", "_", "_"],
    5 : ["_", "_", "_", "_", "_"],
    4 : ["_", "_", "_", "_", "_"],
    3 : ["_", "_", "_", "_", "_"],
    2 : ["_", "_", "_", "_", "_"],
    1 : ["_", "_", "_", "_", "_"]
}

colorList = {
    6 : [39, 39, 39, 39, 39],
    5 : [39, 39, 39, 39, 39],
    4 : [39, 39, 39, 39, 39],
    3 : [39, 39, 39, 39, 39],
    2 : [39, 39, 39, 39, 39],
    1 : [39, 39, 39, 39, 39] 
}

def boardprint():
    global board, colorList

    final = []
    for x in range(len(board)):
        spots = [f"\033[{col}m{board[x+1][idx]}\33[0m" for idx, col in enumerate(colorList[x+1])]
        l = ' '.join(spots)
        final.append(f"| {l} |")
    print('\n'.join(final))
    

    return colorList



guess = ""
result = {}

def checkRight(guess, word, row):
    global board, colorList

    GuessCheckDouble = {i:guess.count(i) for i in guess}
    WordCheckDouble = {i:word.count(i) for i in word}
    result = {key: GuessCheckDouble[key] - WordCheckDouble.get(key, 0) for key in GuessCheckDouble.keys()}
    updateRow = []
    colorRow = []

    for x in range(len(word)):
        if guess[x].upper() == word[x]: # green
            
            letter = guess[x]
            updateRow.append(letter)
            colorRow.append(32)

        elif guess[x].upper() in word and result[guess[x]] == 0: # yellow
            
            letter = guess[x]
            updateRow.append(letter)
            colorRow.append(33)
            
        else: # grey

            letter = guess[x]
            updateRow.append(letter)
            colorRow.append(30)

    

    board.update({row : [updateRow[0], updateRow[1], updateRow[2], updateRow[3], updateRow[4]]})
    colorList.update({row : [colorRow[0], colorRow[1], colorRow[2], colorRow[3], colorRow[4]]})

    winRow = [32, 32, 32, 32, 32]
    if colorRow == winRow:
        boardprint()
        print("YOU WON!")
        exit()



    return board

word = word.upper()
word = list(word)


print("hello! welcome to \033[32;1mWORDLE\033[0m")
print("\tTry to guess a 5-letter word in 6 guesses or less.\n\t\tIf a letter is \033[32mgreen\033[0m, then it is in the right place,\n\t\tIf a letter is \033[33myellow\033[0m, then it is in the wrong place, \n\t\tif a letter is \033[30mgrey\033[0m, then it is the wrong letter. \nREMEMBER\n\t - \033[1mNO\033[0m proper nouns, \n\t - \033[1mNO\033[0m punctuation, hyphens, or accent marks.")
print()
boardprint()
print()

numList = ["FIRST", "SECOND", "THIRD", "FOURTH", "FIFTH", "SIXTH"]
z = 0
while z < 6:
        print()
        guess = input(f"{numList[z]} GUESS  > ")
        guess = guess.upper()
        guessL = guess.lower()
        guess = list(guess)
        if len(guess) == 5 and guessL in lines:
            checkRight(guess, word, z+1)
            boardprint()
            z += 1
        else:
            print("Please enter a valid word.")
            print()
            boardprint()
        
        if z == 6:
            wordStr = "".join(("".join(item) for item in word))
            print("\tYou \033[1mLOST\033[0m lol! The word was", wordStr.lower(), "... (Loser)")
            exit()
\$\endgroup\$
3
  • \$\begingroup\$ One minor thing is that you don't need {row: [updateRow[0], …]} just use {row: updateRow}. \$\endgroup\$ Aug 16 at 12:27
  • \$\begingroup\$ Thank you, that makes sense, but when I tried it and ran the program, the format of the board was weird when it updated the row, so I just left it as is. \$\endgroup\$
    – edwardvth
    Aug 17 at 17:52
  • \$\begingroup\$ Never mind, I went back and tried it again because I was confused about how it didn't work, but I realized it wasn't working because I wasn't doing the same to the line below for colorRow. Thanks! \$\endgroup\$
    – edwardvth
    Aug 17 at 19:59

2 Answers 2

35
\$\begingroup\$

I appreciate you came here as a beginner and you're asking on how to improve. Not every developer wants to get feedback.

I want to know if the way I programmed this game is efficient

IMHO there is no need to worry about efficiency in this program. The main delay/bottleneck is the input of the user.

Do not confuse efficiency with effectiveness. Efficiency is about stuff done per time. Effectiveness is whether the firework pops and makes an effect. You can have many fireworks done per time, but if it does not pop, nobody is impressed.

As a beginner, always go for effectiveness. Premature optimization is the root of all evil, Donald Knuth said.

[...] solid, and overall good code.

I assume that you are learning at free will, which is the best. So I will do as thorough of a review as I can, so that you can make progress faster.

Main method

I wanted to do a top down review, starting at the highest level possible, so I was looking for a main() method - but did not find one.

For me as a reader of the code, I'd like to see the if __name__ == "__main__": pattern somewhere. That's typically easy to spot and somewhat expected in Python programs.

Instead, some code is at the top of the file and some code is between methods and some code is at the bottom of the file.

Well, so let me start reading from beginning to end instead...

Naming

You have a variable called lines which reads a file called words. Is each line a word? If so, you can call the variable words instead.

Hint: use an IDE that has Refactoring features, like PyCharm (go to the name and press Shift+F6).

Consistency

From line 101 (if len(guess) == 5 and guessL in lines:) I see that it's expected that the file contains all words in lower case letters. Otherwise the in check will not work.

In line 5 (word = word.upper()) and line 98 (guess = guess.upper()) you make the words upper case. To me, this adds unnecessary complexity. IMHO, Worlde games are upper case, so just stick to upper case and things will be easier.

words = open("5_letter_words.txt", "r").read().upper().splitlines()
word = random.choice(words)  # upper is here ----^

Pattern initialization

The initialization of the board pattern and colorList pattern seems to violate the DRY principle of clean code. You can try a dictionary comprehension.

I also found it strange to see the patterns begin at 6 and then decrement. The order in a dictionary is irrelevant, so starting at 1 seems more natural. Programmers might find it even more natural to start at 0.

If you start at 0, you might even get rid of the dictionary and make it a list instead (I can't tell at this stage whether it will be beneficial).

Your code essentially becomes a one-liner:

board = {n+1: ["_"]*5 for n in range(6)}
colorList = {n+1: [39]*5 for n in range(6)}
print(board)
print(colorList)

Warning: be aware of unwanted side effects when multiplying lists.

Globals

The globals in line 27 (global board, colorList) impose an unnecessary dependency between your main code (outside of boardprint()) and the method. You can never use boardprint() for anything else than printing exactly board with colorList.

Make these two parameters to the function.

def boardprint(board, colorList):

Complex list comprehension

In line 31, you have a list comprehension, but it interleaves with other complex code, like the color formatting.

Yes, before I have told you to use list comprehension to avoid duplication. This time I want you to not use list comprehension because of separation of concerns.

This line of code has 2 purposes: formatting in color and looping. Separate them.

        for idx, col in enumerate(colorList[x + 1]):
            spots += f"\033[{col}m{board[x + 1][idx]}\33[0m"

Mixture of loop with list comprehension

In boardprint() I can only see a single for loop.

For a 2D board, I expected to see a nested loop, looping over x and y.

So, convert the list comprehension into a loop:

        for idx, col in enumerate(colorList[x + 1]):
            spots += f"\033[{col}m{board[x + 1][idx]}\33[0m"

Wait... that's the same code as before! So you have 2 reasons to do it!

Unused return value

boardprint() has a return value in line 36, but it's never used. Remove it.

Type hints

The method definition in line 43 (def checkRight(guess, word, row):) leaves it open what the type of the parameters are.

Is guess the number of the guess (guess 1, 2, 3 ...) i.e. an integer or is it the guessed word, i.e. a string? Type hints will help the reader understand what it might be.

def checkRight(guess: str, word: str, row: int):

Naming (again)

Even when knowing the type, the method signature is still not clear to me:

def checkRight(guess: str, word: str, row: int):

Better names would be guessedWord and correctWord. At this time in the review, I have no clue what row is good for. Hopefully I'll find a better name for that as well.

Method doing more than one thing.

Line 44 (global board, colorList) makes use of globals once more. Make it parameters as well. But it's starting to be many parameters already. Maybe this method is doing too many things.

checkRight is from line 44 to 80. That's too long. It's another indicator that the method does too much. Let's see...

It seems to me that the method should decide whether characters become grey, yellow or green. However, it's not only doing that, but also

  • updating the board and

  • updating the colors and

  • deciding whether the game is over

Split this method into 4 smaller methods and it'll be much simpler to read, understand and debug.

Magic numbers

checkRight() has a few constants that are related to colors. We call such numbers "magic numbers", because they appear out of nothing with no semantics. The color comment is far from the usage (at least 4 lines).

We typically give them names. Since they are constants, we agree to make them upper case:

GREEN = 32
YELLOW = 33
GREY = 30

Likewise you can do that for the initial color (whatever color 39 is).

The number 6 in line 8 (initialization of board), line 17 (initialization of colorList), line 95 (while z < 6) and line 110 (if z == 6:) seems to have a special meaning as well. Either replace it by a constant as well, or perhaps by an expression instead (e.g. len(board)).

Very long line

Line 88 is more than 300 characters long. It's quite hard to figure out what will be printed. Take advantage of multiline strings like

print(
    """
    \tTry to guess a 5-letter word in 6 guesses or less.\n
    \t\tIf a letter is \033[32mgreen\033[0m, then it is in the right place,\n
    \t\tIf a letter is \033[33myellow\033[0m, then it is in the wrong place, \n
    \t\tif a letter is \033[30mgrey\033[0m, then it is the wrong letter. \n
    REMEMBER\n\t - \033[1mNO\033[0m proper nouns, \n
    \t - \033[1mNO\033[0m punctuation, hyphens, or accent marks.
    """
    )

That way it will be immediately obvious what will be printed.

Move code into methods

The welcome message and instructions could make a new method called instructions() or so.

Start indexing at 0

Lines 93 to 95 use 0-based indexing:

numList = ["FIRST", "SECOND", "THIRD", "FOURTH", "FIFTH", "SIXTH"]
z = 0
while z < 6:

But before we had 1-based indexing for board and colorList. So, a 1-based index does not seem to be a general style here. Therefore I tend to start at 0. This will also simplify line 31 (enumerate(colorList[x + 1])) and that part of the code gets rid of the magic 1.

Value the user

In real software development, the user will pay for the software. Try to value him. A statement like

LOST lol! The word was ... (Loser)

may be the trigger that this customer never buys something from you again. Perhaps the user is only 8 years old and does not know many words yet. Perhaps the user is 93 and has made best efforts on solving the puzzle while having Alzheimer. No need to laugh at them or make fun of them.

Some more stuff I found while doing the refactoring myself...

  1. I noticed that there is an unnecessary word = word.upper() in line 83

  2. Converting the word into a list of characters in line 84 (word = list(word)) is confusing. Just let it be a string. You can already iterate over a string. This also saves you from converting it back into a string in line 111 (where the join is done twice, for some reason):

wordStr = "".join(("".join(item) for item in word))

    4. There are 4 places where printboard() is called. Once the code is sorted a bit, you hopefully see that it's easy to reduce that to 2 (or even 1).

  1. I'm not a native speaker, but I renamed numList to ordinals, because it's not a list of numbers, but a list of counted numbers.

  2. guess = "" is unnecessary, because it's assigned at input() anyway

  3. result = {} can be removed, because it's not used.

  4. if guess == word: is a more trivial way to check for a win instead of comparing colors

winRow = [32, 32, 32, 32, 32]
if colorRow == winRow:
  1. The length check if len(guess) == 5 is redundant as long as all words in your word list have 5 characters.

  2. In checkRight(), all paths of the if/else contain the same code, so you can put these lines out of the if/else:

letter = guess[x]
updateRow.append(letter)
  1. guess[x].upper() within checkRight(): the upper() is redundant, since the user input has already been converted to upper case. guess[x] is equivalent to letter after point 10.

  2. Instead of having updateRow in checkRight() and later update board by updateRow, update board directly.

  3. Same for colorRow.

Oof...

Well, that was a lot, wasn't it? Actually I don't exactly know how your code looks like, after you apply all suggestions.

Is it worth the effort? Certainly! Go through the code, make it better and compare the new code to the old code. I went from 113 lines to 74, which is 35% reduction of code. It's also much easier to make changes to it.

Is the code perfect? Not yet. You might want to keep on learning and get feedback again.

At some point, you may want to learn about classes and objects, which can give you a totally new programming style. But that would be too much for now. Object oriented programming will allow you to combine board and colorList into a single unit, so that it's impossible to make them different of sizes. The board might be able to print itself, etc.

\$\endgroup\$
4
  • 1
    \$\begingroup\$ Lol absolutely true that software should respect the user. \$\endgroup\$
    – qwr
    Aug 16 at 7:26
  • 5
    \$\begingroup\$ One thing perhaps worth noting is that it's more common in Python for naming to be done using snake case. So check_right may be more appropriate than checkRight for example. Of course, if you're consistent, that might not be a big deal. But I prefer to try to stick the conventions of the language I'm using. \$\endgroup\$ Aug 16 at 12:25
  • \$\begingroup\$ Thanks for your reply, it definitely helped! But one thing that I don't understand is the part about your consistency section where you make all the words in the file all uppercase. Why should I do that if I'm only using one word from the whole file? Shouldn't I just make the randomly selected word uppercase instead of making the whole file uppercase? \$\endgroup\$
    – edwardvth
    Aug 17 at 21:18
  • \$\begingroup\$ @edwardvth: In the (original) line if len(guess) == 5 and guessL in lines: you compare the word entered by the user to all words in the "dictionary". If the user enters a word that gets converted to uppercase, all other words need to be uppercase too, so that the comparison will work. \$\endgroup\$ Aug 18 at 7:30
4
\$\begingroup\$

What immediately stood out to me was strings using ANSI terminal escape codes manually. I recommend writing a function that applies the proper escape codes for colors and formatting, which will make your code easier to maintain and understand (what will \033[32;1mWORDLE\033[0m look like to the user?) Better yet, you can use a library like ansicolors that does all that for you.

\$\endgroup\$
1
  • 3
    \$\begingroup\$ Alternative libraries include colorama, termcolor and blessed. Personally, I prefer blessed because it has functions to measure the length of the text without escape sequences. It can be useful in some situations. It also has other things like keyboard handling. \$\endgroup\$ Aug 16 at 14:30

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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