On Tue, Oct 14, 2014 at 7:04 PM, Revenant <faceofoblivionoffic...@gmail.com> wrote: > As previously stated, I am new to Python and would also like to see if any of > you programming gurus have some suggestions about how I can simplify code, > and also if there are any other good starter programs to work on to improve > my skills. >
Hi! Happy to help out. But first, there's one meta-suggestion that I'd like to make: Use something other than Google Groups. As shown by the line above, Google Groups has some issues with line breaks and text wrapping; and it gets much worse when you reply to someone else's post. I suggest using the mailing list instead: https://mail.python.org/mailman/listinfo/python-list Also, it can be helpful to state what version of Python you're using, and on what platform. Your "press any key to quit" request suggests you're probably invoking the script using a GUI, quite possibly Windows, but that's far from certain; and as Dave's response shows, the solutions depend on platform. I'm presuming you're using some 3.x version of Python, as you use input() and print() functions, but I can't tell whether you're on 3.2, 3.3, 3.4, or even an unreleased 3.5. It's not likely to make a huge amount of difference with a script this simple, but as a general rule, more information is better than less. So, on to the code! > # Creates the main menu. > def menu(): > # Sets the scores to 0. > global playerscore > global compscore > global draws > playerscore = 0 > compscore = 0 > draws = 0 You're importing these globals into several places that don't need them. The menu shouldn't have to concern itself with these scores; you could initialize them all to zero at top-level, and then keep the different functions more self-contained. The displaying of the menu doesn't logically involve zeroing out the scores; imagine making the very slight (and quite logical) change of having the full menu redisplayed after each game, instead of just having the "Again? (Y/N)" prompt - suddenly your scores aren't getting carried over. > menuselection = input('Please enter a selection: (Play/Help/About): ') > # Checks for an invalid selection. > while menuselection != 'Play' and menuselection != 'play' and > menuselection != 'Help' and menuselection != 'help' \ > and menuselection != 'About' and menuselection != 'about': > print('You have entered an invalid selection.') > menuselection = input('\nPlease type a selection: (Play/Help/About): > ') > else: > if menuselection == 'Play' or menuselection == 'play': > play() > elif menuselection == 'Help' or menuselection == 'help': > instructions() > else: > about() The "else" clause on a while loop isn't necessary here, because you're not using break. All you need is to put your code outside the loop. Alternatively, you can combine the verification and iteration into one pass, which reduces redundancy and makes it easier to add more options. I'm also incorporating some suggestions already made, including Dave's dispatch dictionary. func = {"play":play, "help":instructions, "about":about} while True: menuselection = input('Please enter a selection: (Play/Help/About): ').lower() if menuselection in func: break print('You have entered an invalid selection.') func[menuselection]() Note how the loop condition is now inside the loop ("if ...: break"), with the while statement simply creating an infinite loop ("while True:"). > def play(): > # Player chooses Paper, Rock, or Scissors. > playerselect = input('\nPlease choose Paper, Rock, or Scissors: ') Try to avoid comments that simply state what the next line(s) of code do. At best, they're redundant; at worst, they're confusing, because they can get out of sync with the code. Imagine adding a fourth option (Claw Hammer) and forgetting to change the comment. (Why claw hammer? http://www.theregister.co.uk/2003/06/09/the_bastard_interviewer_from_hell/ ) > import random It's much more common to put your imports at the top of the script, rather than inside a function. > # Creates the instructions. > def instructions(): > print('\nPaper, Rock, Scissors is a simple game played against a computer > opponent.') > print('The player will have a choice of selecting paper, rock, or > scissors.') > print('The player\'s result will be compared with that of the computer to > determine who wins the round.') > print('In the event that both the player and the computer have the same > selection, the round will end in a tie.') > print('\nPaper beats rock but loses to scissors.') > print('\nRock beats scissors but loses to paper.') > print('\nScissors beats paper but loses to rock.') > print('\nGood luck, and have fun!\n') > menu() As noted elsewhere, you have mutual recursion happening here. That's not an advised technique in Python, as you can blow your stack (there's no tail call optimization here). Much more common would be to have self-contained functions that perform one job (in this case, displaying the instructions on the console), and have looping done in the places that want looping - probably inside menu(). Incidentally, this function doesn't *create* anything. Your comment (which would be better as a docstring, fwiw) is slightly confusing here. > # Creates the about section. > def about(): > print('\nPaper, Rock, Scissors\n\nVersion 1.0\n\nCreated by <Name>, 07 > October 2014\n') > menu() Huh, I didn't know your name was actually <Name> :) > # Calculates score. > def score(): > print('\nCurrent Scores: ') > print('\nPlayer Score:', playerscore) > print('\nComputer Score:', compscore) > print('\nDraws:', draws) > > > # Start of program operations. > print('Welcome to Paper, Rock, Scissors!\n') > menu() Suggestion: Where possible, follow a general policy of "Define Before Use". For any given global name (like function names), the first occurrence in the file should be its definition, and all usage should be lower in the file than that. Once you clean up the mutual recursion, you'll be able to lay your code out like this; there'll be a single menu() function near the bottom, and functions like score() will be much higher up, as they have almost no dependencies. It'd look something like this (function bodies and blank lines elided for brevity): import random playerscore = compscore = draws = 0 def instructions(): def about(): def score(): def again(): # which will return True or False, as per Dave's suggestion def play(): def menu(): print("Welcome") menu() When someone comes looking at a program like this, s/he can see exactly what depends on what. There can be exceptions, of course (sometimes mutual recursion really is the right thing to do, especially with tree-walking routines), but those can then be noted with comments ("Mutually recursive with xyz()."), or in some cases may be obvious from the names. In any case, the bulk of most programs can be written in this style, and it does improve clarity. Hope that's of value. Like all advice, it's just one person's opinion, and you're most welcome to reject it; but you did ask for code review, so I'm hoping you'll at least know *why* you're rejecting any piece of advice :) All the best! ChrisA -- https://mail.python.org/mailman/listinfo/python-list
