I am about to graduate with my Associates degree in Math and will soon go for my bachelors. I've decided with two years of school left its best if I start learning to program. I am starting with python.
I recently completed a beginners course on YouTube from free code camp. I completed novice programs that come with the course. (Guessing game)(mad Libs) & a(multiple choice quiz)
Now I am moving on to complete 3 programs outside of the lecture before I move to another python course. My ideas were (Dice game), (Hangman), (short game)
Below is my python code for the dice game. I planned on creating an array and looping through then selecting a random number from the loop. It's proving a little difficult. so far I just print a 3x3 matrix then select a random number. fyi the 3x3 is suppose to represent a 9 sided dice.
My question: Is this code good enough for me to move on to my next program or should I stick with it and code the dice game the way I originally planned. Thank you for the feedback.
#Create dice using matrix 9 sides
#Create function: Give user option to roll dice
#Return random dice number 1-9
from random import seed
from random import randint
dice = [
[[1],[2],[3]],
[[4],[5],[6]],
[[7],[8],[9]]
]
def diceroll():
start = input("do you want to play dice Y/N ")
if start == "Y" or start == "y":
print(dice[0])
print(dice[1])
print(dice[2])
x = input("Do you want to roll the dice: Y/N ")
while x == "y" or x =="Y":
if x != "y" or x !="Y":
for i in range(1):
roll=randint(1,9)
print(roll)
x = input("Do you want to roll the dice again: Y/N ")
else:
print("GoodBye: ")
1 Answer 1
A bit of honesty
When starting to review your code, I found it unintelligible and I was unable to follow the logic. I was able to understand what you are trying to do but not your implementation so I will post a few improvements here.
Structure
There is no need for the for
loop in the body. You can see this as you do not use the value you gain from the interand. Because of this, we can cut the loop out.
.lower()
We can strip checking for a lowercase or uppercase answer by using .lower()
. This takes a string and returns the all lowercase equivalent. This means we can strip the user input check to: if start.lower() == "y":
Walrus operator
New in Python 3.8 is the walrus operator! It looks like this :=
. We can use this in the while
loop as it can handle prompting the user to quit. We can combine this with the .lower()
to simplify the while loop.
Unused import
Despite importing random.seed
you never use it, we can remove it from the imports.
Final code
from random import randint
dice = [
[[1],[2],[3]],
[[4],[5],[6]],
[[7],[8],[9]]
]
def diceroll():
"""Loop over dice and print the value"""
start = input("do you want to play dice Y/N ")
if start.lower() == "y":
print(dice[0])
print(dice[1])
print(dice[2])
while (x := input("Do you want to roll the dice? [y/n]: ").lower()) == "y":
roll=randint(1,9)
print(roll)
else:
print("GoodBye: ")
Further improvements
The function uses a global variable which is a bad practice, it is much better to give the function the global variable as an argument. I did not know if you wanted the dice array for something else, so I left it as it is. Also I question the need for a dice array in the first place, but again, this is your code with your own spec.
-
\$\begingroup\$ Thank you for taking out time to give me feedback. I'll get better at this as I practice more. I will study the logic of the code you gave me. Should I code this again or should I move on? \$\endgroup\$JeremiahV– JeremiahV2020年04月17日 08:57:49 +00:00Commented Apr 17, 2020 at 8:57
-
\$\begingroup\$ I recommend just playing around with my code, maybe try to implement the argument idea and represent the general case of any sided dice as an argument. Once you feel like you understand the code and can explain the logic, then I would move on. \$\endgroup\$EugeneProut– EugeneProut2020年04月17日 09:01:55 +00:00Commented Apr 17, 2020 at 9:01
-
1\$\begingroup\$ The walrus operator is a cool thing, but in this code it can be remove completely. The input value is not used again. \$\endgroup\$The Librarian– The Librarian2020年04月17日 13:05:02 +00:00Commented Apr 17, 2020 at 13:05
-
\$\begingroup\$ This is a great revision, but I agree with Librarian's point above on getting rid of
x
. My two cents is to replacedice
with the actual multiline string you want to print (do some ASCII art or whatever, between"""
marks), since the numbers themselves aren't being used for anything. If it's a single string you can just print it asprint(dice)
. Having it as a global is fine IMO as long as it's a constant, but give it a name likeDICE_ASCII_ART
or something (allcaps to indicate it's a global constant). \$\endgroup\$Samwise– Samwise2020年04月17日 15:58:43 +00:00Commented Apr 17, 2020 at 15:58
Explore related questions
See similar questions with these tags.
while
loop and theif
-statement inside of it are incompatible; only one can be true). \$\endgroup\$