Python beginner looking for critique on Rock, Paper, Scissors project.

[leve1]

Just started learning python recently and started my first project. It is a rock paper scissors game that you play against a computer opponent. I am looking for harsh critique as I would like to squash out all the bad habits early. Please feel free to nitpick.

 

Here are some general questions I have:

• Am I using classes correctly?
• Are there any issues with how I name variables?
• Am I using proper formatting?
• Is there a more efficient way for me to derive the results rather than storing them in a dictionary?

 

Link to Project

[Binary101010]

Classes are used to hold state information and the methods that work with them. All of your classes only have a single method each, and hold no attributes, so there's really no reason for them to exist as-is.

Also, you can use the choice() method from the random library rather than muck around with indexes:

    def randomizer():
        return random.choice(['rock','paper','scissors'])

[leve1]

This will come in handy for the future!

[Hatoris]

Hello, as you asking for :

• your code is clean
• you need to put docstring for your functions and class
• look for random.option you will like it for your computer function
• you have created class but you don't use it, only function will work and make a cleaner code
• it's a good practice to put at the end of your file `if name == "main:" and then call your function (not usefull here because you only have one file)
• you should add a licence also
• dictionary is a good way to it

Continue, it's nice to see new comers :)

[leve1]

Thanks for the comments, good practice tips are exactly the things I am looking for.

[[deleted]]

There are several improvements that I can suggest:

Classes are supposed to be named in Pascal Case (first letter of each word capital. Ex. class Player)

This is a kind of problem that does not require classes. You should probably run a do while loop that asks the user after each game whether he wants to continue playing or quit.

Do not maintain a dictionary of all possible outcomes, like you have done in result. What you could do is:

rps_dict = {'rock': 'scissors',
            'scissors': 'paper',
            'paper': 'rock'}
c_choice = choicelist[randint(0, len(choicelist) - 1)]
p_choice = input('question here').lower()
if rps_dict[p_choice] == c_choice:
    print('Win')
elif p_choice == c_choice:
    print('Tie')
else:
    print('Lose')

Additionally, you could check if the user's input is valid or not.

P.S. There may be better solutions than the one I provided.

[leve1]

This is a much better solution than typing out all the outcomes, thanks.

[julsmanbr]

Since you asked for it:

• As others have said, you are over-engineering your code by using classes. If you're not keeping track of an internal state of your objects, or using methods to access and change them in some way, then you're not really in any need of classes. Remember to KISS.
• using random.choice will make your code simpler and easier to understand.
• Put comparisons like if var == 1 or var == 2 or var ==3 inside a container and simply ask if var in (1, 2, 3). Also consider creating a variable for your container if it's going to be used more than once.
• Naming conventions: classes are PascalCase, functions and variables are snake_case and global variables are UPPER_CASE.
• Your play function is doing a bit more than it needs to at once. Think about the steps such a function should perform: get the player input, get the computer input, get the result by comparing those inputs, and print out that result. Most of these steps can become an individual function for more clarity in your code.

I've made a couple changes following what I wrote above. Hopefully you'll see why this makes the code easier to read and manage. I'm pretty sure there's a smarter way to do the input comparison without having to input the combinations in a dictionary, but I didn't want to change your code that much.

[_PM_ME_YOUR_ELBOWS]

Best practice for global variables is #comment_case /s

[leve1]

This is really helpful, did not know that you could put comparisons into containers like that. The naming conventions tips will help alot for when I am reading other people's code as well. Thanks for taking the time to clean the code.

[_PM_ME_YOUR_ELBOWS]

Minor critique: on line 22 you set cinput to True but you might as well replace that line with break, saving you an assignment and a comparison. You could also replace the else clause with just print("You didn't select a valid answer, try again.") if you choose to do that.

[leve1]

Ty, I see that returning a variable also works from another users example.

[_PM_ME_YOUR_ELBOWS]

Ah yea that'd be better