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
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...
I noticed that there is an unnecessary word = word.upper()
in line 83
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).
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.
guess = ""
is unnecessary, because it's assigned at input()
anyway
result = {}
can be removed, because it's not used.
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:
The length check if len(guess) == 5
is redundant as long as all words in your word list have 5 characters.
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)
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.
Instead of having updateRow
in checkRight()
and later update board
by updateRow
, update board
directly.
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.
{row: [updateRow[0], …]}
just use{row: updateRow}
. \$\endgroup\$