Simple Python Console Game With Inefficient Code Flow

Question

I made a simple program as an exercise in improving my code flow in Python, which does the following:

1. asks user to input their name
2. presents them with a choice to either print their name in reverse, or print the number of characters
3. after the output is printed, the user is asked if they want to "play again". if not, exits

Have I done anything especially goofy? Any low hanging fruit? Best practice I'm unaware of?

from sys import exit

def userinput():
    print('Enter your name:')
    n = input()
    print('Hello, ' + n + ', would you rather (R)everse or (C)ount the letters of your name?')
    a = input()
    return (a,n)

def choosef(choice,myname):
    while choice not in ['R','C']:
        print("Only 'R' or 'C' are valid. Try again:")
        choice = input()
    if choice == 'R':
        print(''.join(reversed(myname)))
    else:
        spaces = 0
        for c in list(myname):
            if c == ' ':
                spaces = spaces + 1
        print('Total # Characters: ' + str(len(myname) - spaces))
    playagain()

def playagain():
    print("Would you like to play again, (Y)es or any other character to exit?")
    yn = input()
    if yn == 'Y':
        ui = userinput()
        choosef(ui[0],ui[1])
    else:
        exit()

ui = userinput()
choosef(ui[0],ui[1])

Answer 1

Try to forget everything that you know about the code, and assume that you aren't the author. Imagine yourself to be a third person reading the code, he/she knows nothing about it and just reading it for the first time.

ui = userinput()
choosef(ui[0],ui[1])

Alright, userinput() must take some input and put it into ui. Makes sense, but what is choosef()?.

Naming convention

Use meaningful names for functions and variables. take this example

x = 3.14
y = 24

what is x and y to the reader? Is that the price of something?

pi_value = 3.14
number_of_hours = 24

Ahhh okay, now I know exactly what 3.14 means. So If I use it somewhere the reader will also know what I exactly mean. This is an important part of writing clean code.

Another aspect is the style. There are a few ways I can write user input*

user_input()
USERINPUT()
UserInput()
userinput()

Which one should I follow?

To have a consistent naming convention, python code follows the PEP-8 naming convention. When I say follows, I mean that it is recommended that you should follow it too as other python libraries also use this. It makes the code look cleaner.

Things like functions follow : lower_snake_case

Classes follow: CamelCase

You can read the link for more information.

Taking input in Python

print('Enter your name:')
n = input()

Clearly, you want to display a message to the user before he enters something. This is why the input() function has something called input prompt.

name = input("Enter your name: ")

You can display the message between the (). This removes the additional line. Also note that I changed n to name for the reasons I mentioned above.

Formatting strings

From your code, I can see that you concatenated strings with + to form meaningful sentences. That works but there is a huge problem when you want to use variables of different types.

name = "Eric"
age = 14
job = "Comedian"
print("Hello " + name  + "You are " + age + " years old and you are a " + comedian)

TypeError: can only concatenate str (not "int") to str

Simply use Python 3's f-strings. Place the letter 'f' before "

print(f"Hello {name}, you are {age} years old ")

Cleaner.

return value from userinput()

What you are currently doing is returning a tuple of choice and name. It's better that you keep them as they are because later on, you slice from it just to get them again. This means you directly do

return name, choice

# Calling the function


name, choice = userinput()

---

Code structure

Couple of points

• Do not ask "Do you want to play again? " in the play_again() function. The reason you called that function should be because the user wants to play again. Move that to a playgame() function which will call user_input() every time the user wants to play the game, and break from the loop only if the user enters 'n' or "no"

• spaces = spaces + 1 can be simplified into spaces += 1

• Move the part where you count the number of characters into a separate function which will return an integer. So your play_game() function doesn't do anything other than play the game. When you need the characters , number_of_char = character_len( name ).

• Use enum for clarity.

An improved version of the code

from enum import Enum

class PrintChoices(Enum):
    number_of_char = 'c'
    reversed_name = 'r'
    exit_choice = 'e'


def find_num_of_char(name):
    return len(name) - name.count(' ') 

def reverse_name(name):
    return name[::-1]

def user_input():
    name = input("Enter your name: " )
    choice = input("Would you like to (r)everse your name\n or would you like to print the number of (c)haracters or (e)xit?: ")
    return name, choice

def clear_screen():
    print(chr(27) + "[2J")

def play_game():
    while True:
        clear_screen()
        name, choice = user_input()
        if choice == PrintChoices.reversed_name.value:
            print(reverse_name(name))
            input("Press any key to continue...")
        elif choice == PrintChoices.number_of_char.value:
            print(find_num_of_char(name))
            input("Press any key to continue...")
        elif choice == PrintChoices.exit_choice.value:
            break
        else:
            input("Invalid input, Press any key to continue...")



play_game()

Note: I have also added print(chr(27) + "[2J") to clear the screen.

Answer 2

You can have a better structure. Here is detailed explanation. Data Flow Errors which you should not do in real world:

• Here, your function calls don't make sense. Whenever you define and use functions, define them in such a way that they don't call each other i. e. there should be tree structure. Main function will control all. (See Below Code to get glimpse.)
• Your script can have better structure. See Below Code.

*** PROTIP: If you want to make your code more maintainable, break it into modules and put all of them in different files and import them ***

def take_name_input():
    print('Enter your name:')
    n = input()
    return n

def ask_choice():
   print('Hello, ' + n + ', would you rather (R)everse or (C)ount the letters of your name?')
    a = input()
    return a

def playgame(myname):
    while True:
        choice = ask_choice()
        if choice == 'R':
            print(''.join(reversed(myname)))
        elif choice == 'C':
            for c in list(myname):
                if c == ' ':
                    spaces = spaces + 1
            print('Total # Characters: ' + str(len(myname) - spaces))
        else:
            break

name = take_name_input()
playgame(name)

Answer 3

I think the structure is quite convoluted, with calls to input() and choosef() scattered in multiple places/levels, thus the program is not super readable and not scalable (if you had to handle more user inputs).

A more readable approach would be a main loop like - in pseudo code

while True:
  Get user input from 3 choices R / C / E(xit)
  if input = E:
     exit
  else if input = R:
     handle R case
  else 
     handle C case

Also in python there is a shorter way to reverse a string with slicing syntax: "abc"[::-1]

Answer 4

There are two area's where your code could improve further.

First you can run tools like flake8 or pylint to check your code for 'violations' of PEP8. PEP8 is a Python style guide that is accepted as the de facto style for Python code. I won't focus on this since this mostly an automatic process and a somewhat personal choice.

Secondly you can make some improvements to your code with regards to naming and documentation. This is what we call the readability of your code. I've taken a shot at this myself and this is what I (personally) find to be better code.

One thing you can improve is removing all magic from your code. In the two lines below you set a variable ui and then pass the first and second element to another function. If I see these two lines, I have no idea what you're doing.

ui = userinput()
choosef(ui[0],ui[1])

This can be simplified and made more readable by selecting better variable names and (in this case at least) using tuple unpacking:

action, name = get_user_input()
handle_user_action(action, name)

The example above can be applied throughout your code. I always judge my code (and the code I review) with the question: "If I show this block of code to someone, is the intention of the code clear to the person reading it, without requiring knowledge of the entire program?"