Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I am sort of new to programming. I would like to understand the code areas that are garbage and could be better written keeping the exact (if possible) behavior the code has now.

from random import randint
import time
from collections import OrderedDict

cast_again = 'r'
statistics = OrderedDict()
casts = 0
menu_options = """
> Options:
>    R - to cast again;
>    S - view stats;
>    E - exit the programme.
> """

def rollDice(dice):
    input('> Press Enter to cast dice!')
    time.sleep(1)
    cast_result = []
    for a_cast in range(0,dice):
        cast_result.append(randint(1,6))
    return cast_result

def atLeastALetterInput(text_to_display):
    cast_again = 'x'
    try:
        cast_again = input(text_to_display)[0].lower()
        return cast_again
    except IndexError:
        print('> Please make sure input is valid before pressing enter...')   

while 1: 
    try:
        num_dice = int(input('> Enter the number of dice you would like to cast! (max 10):'))
        if num_dice not in range(0,11):
            print('> Please enter a value between 1 and 10! ')
            continue
        break
    except ValueError:
        print('> Non-numerical characters detected. Try again!')

while 1:
    if cast_again not in ('r', 's', 'e'):
        print('> Ups! Available options R(new cast), S(statistics) and E(exit).')
        cast_again = atLeastALetterInput(menu_options)
        continue   
    if cast_again == 'r':
        next_cast = casts + 1
        print('> Here comes cast number {}'.format(next_cast))
        casts +=1
        cast_results_to_stats = rollDice(num_dice)
        statistics[casts] = cast_results_to_stats
        print('> Here is your result: {}'.format(cast_results_to_stats))
        cast_again = atLeastALetterInput(menu_options)
    elif cast_again == 's':
        for key in statistics:
            print('> *** Cast {} resulted in {}'.format(key, statistics[key]))
        cast_again = atLeastALetterInput(menu_options)
    elif cast_again == 'e':
        print('> The game finished after {} casts.'.format(casts))
        break

print('> Good bye! ')
input()
share|improve this question
1  
Reposted from Stack Overflow –  200_success Aug 11 at 5:50
    
Please remember to upvote all answers you find helpful. –  200_success Aug 11 at 14:52

4 Answers 4

up vote 6 down vote accepted

About rollDice

The name of the function does not follow PEP 8, the Python style guide.

The name of the parameter is not that great.

You don't need 0 as a first parameter of range as it is the default behavior.

The value a_cast is not used. Usually, the name for throw-away values in Python is _.

The loop could be written in a list compregension.

After taking into account these details, your code looks like :

def roll_dice(nb_dice):
    input('> Press Enter to cast dice!')
    time.sleep(1)
    return [randint(1,6) for _ in range(nb_dice)]

About atLeastALetterInput

Here again, the name does not seem correct.

The variable is not required.

After rewriting this, code looks like :

def get_letter_from_user(text_to_display):
    try:
        return input(text_to_display)[0].lower()
    except IndexError:
        print('> Please make sure input is valid before pressing enter...')

About your first while loop

It should probably be put in a function with a signature like def get_nb_from_user(min_val, max_val):.

You could remove the need for a continue by re-organising the conditions :

    num_dice = int(input('> Enter the number of dice you would like to cast! (max 10):'))
    if num_dice in range(0,11):
        break
    else:
        print('> Please enter a value between 1 and 10! ')

Checking if an integer is in an interval by checking if it is in the result from range is not efficient at all : if you were to check if a number is between 1 and 100000000, it would take a very long time even though this should be pretty simple.

Your code becomes :

def get_nb_from_user(min_val, max_val):
    while True:
        try:
            nb = int(input('> Enter the number of dice you would like to cast! (max %s):' % max_val))
            if min_val <= nb <= max_val:
                return nb
            else:
                print('> Please enter a value between %s and %s! ' % (min_val, max_val))
        except ValueError:
            print('> Non-numerical characters detected. Try again!')

num_dice = get_nb_from_user(1, 10)

About your second while loop

To keep things simple, you should try to call get_letter_from_user from a single place.

This is quite easy to do :

while True:
    cast_again = get_letter_from_user(menu_options)
    if cast_again == 'r':
        next_cast = casts + 1
        print('> Here comes cast number {}'.format(next_cast))
        casts +=1
        cast_results_to_stats = roll_dice(num_dice)
        statistics[casts] = cast_results_to_stats
        print('> Here is your result: {}'.format(cast_results_to_stats))
    elif cast_again == 's':
        for key in statistics:
            print('> *** Cast {} resulted in {}'.format(key, statistics[key]))
    elif cast_again == 'e':
        print('> The game finished after {} casts.'.format(casts))
        print('> Good bye! ')
        input()
        break
    else:
        print('> Ups! Available options R(new cast), S(statistics) and E(exit).')

Then again, as explained in a different answer, there is no need for an ordered dictionnary as a list would do the trick.

Then your code begins (variable definition included) :

num_dice = get_nb_from_user(1, 10)
statistics = []

while True:
    cast_again = get_letter_from_user(menu_options)
    if cast_again == 'r':
        print('> Here comes cast number {}'.format(len(statistics)+1))
        cast_results_to_stats = roll_dice(num_dice)
        statistics.append(cast_results_to_stats)
        print('> Here is your result: {}'.format(cast_results_to_stats))
    elif cast_again == 's':
        for i, cast in enumerate(statistics):
            print('> *** Cast {} resulted in {}'.format(i, cast))
    elif cast_again == 'e':
        print('> The game finished after {} casts.'.format(len(statistics)))
        print('> Good bye! ')
        input()
        break
    else:
        print('> Ups! Available options R(new cast), S(statistics) and E(exit).')

Now, in Python, it is usually a good thing to move the part of your code actually doing stuff behind a main guard. Thus, your code begins :

def main():
    """Main function"""
    num_dice = get_nb_from_user(1, 10)
    statistics = []

    while True:
        cast_again = get_letter_from_user(menu_options)
        if cast_again == 'r':
            print('> Here comes cast number {}'.format(len(statistics)+1))
            cast_results_to_stats = roll_dice(num_dice)
            statistics.append(cast_results_to_stats)
            print('> Here is your result: {}'.format(cast_results_to_stats))
        elif cast_again == 's':
            for i, cast in enumerate(statistics):
                print('> *** Cast {} resulted in {}'.format(i, cast))
        elif cast_again == 'e':
            print('> The game finished after {} casts.'.format(len(statistics)))
            print('> Good bye! ')
            input()
            break
        else:
            print('> Ups! Available options R(new cast), S(statistics) and E(exit).')

if __name__ == "__main__":
    main()

Then, on top of this, you should try to add docstrings to your functions.

share|improve this answer
    
Thank you Josay! It was exactly what I was looking for: a way to understand what to look for when refactoring. –  user50916 Aug 11 at 20:35

It's not too bad. You're starting to break things into functions and are doing error checking. Some issues:

The first thing I see at the top is a flag which is not named in a way that says what it does, and is not used until near the end of the file. Given that you only have one exit condition in your second input loop and no nested loop, you could just get rid of flag and use break.

You have a function validInput which does not really validate the input, just ensure that at least 1 character was input. In fact you're not validating input per se - for example, after I have selected the statistics option s you don't show me that option again, but s is still accepted. Here you only have 2 slight variations on 1 main menu, but if your menu structure was any more complicated you would need to start factoring your menus into data structures and keeping track of menu state.

That leads me to the options_after_ strings, which duplicate information. First of all, you're duplicating the inputs themselves (i.e. the letters r, s and e). If you changed them, you'd have to change them in all of the strings as well as multiple if statements in your input loop. Second, you're duplicating the formatting between your two options_after_ strings. This is really a bigger issue than the last two.

In order to make this "DRY" (don't repeat yourself) you want each input letter and each menu item description to be in the program exactly once, and then refer back to those definitions from elsewhere. Then, generate the menu text from that data as well.

share|improve this answer
    
Thank you, Jason! Got rid of the flag and renamed validInput to be more suggestive of what it does. Working out on the remaining suggestions :). –  user50916 Aug 11 at 7:24
    
Dropped the 2 variations on menu also. I guess simplicity is the thing to look after. Realized that i could have kept only options_after_cast and reuse it by slicing out the "S - view stats" options so there isn't any actual need for options_after_stats. Regarding: <<First of all, you're duplicating the inputs themselves (i.e. the letters r, s and e). If you changed them, you'd have to change them in all of the strings as well as multiple if statements in your input loop.>> - do you suggest that i should define them as constants? –  user50916 Aug 11 at 7:51

Your code is pretty good and well written. I would just change a bit the inside of "while flag:" by having the part where you test the validity of cast_again at the end as the last case (else).

That is, instead of

while 1:
    if cast_again not in ('r', 's', 'e'):
        # Handler
        …
        continue   
    if cast_again == 'r':
        …
    elif cast_again == 's':
        …
    elif cast_again == 'e':
        …

write

while 1:
    if cast_again == 'r':
        …
    elif cast_again == 's':
        …
    elif cast_again == 'e':
        …
    else:
        # Handler
        …
share|improve this answer

Your time.sleep(1) reminded me of this funny story.

The three basic issues I see are…

Data storage

Why use an OrderedDict, whose keys are sequential integers? It would be simpler to use an array.

Using a casts variable to track the number of casts is redundant, as that would be simply the length of the array.

Prompting

The following procedure happens frequently:

  1. Print a prompt.
  2. Fetch a line of user input.
  3. Transform the user-provided string into the desired form.
  4. Validate it, and display an error message if necessary.
  5. If any of that failed, goto (1).

Unfortunately, the specifics also differ greatly. Here is my attempt to reshuffle the complexity into an ask() function.

from random import randint
from sys import exit
from time import sleep

casts = []

def ask(prompt, xform=None, xform_err_msg='Invalid input', validate=None):
    """Prompts the user for input, repeating until a valid response is
    obtained.

    xform is an optional function to transform the input string; an
    exception during transformation will cause xform_err_msg to be printed,
    and the question will be asked again.

    validate is an optional function to check the (possibly transformed) value.
    It can either return None or True to indicate that the value is valid,
    or return a string as an error message to be printed, in which case the
    question will be asked again.

    On EOF, calls sys.exit()."""

    while True:
        try:
            val = input(prompt)
            if xform:
                val = xform(val)
            if validate:
                msg = validate(val)
                if msg == True:
                    break
                if msg is not None:
                    print(msg)
            else:
                break
        except EOFError:
            print()
            exit()
        except:
            print(xform_err_msg)
    return val

num_dice = ask('> Enter the number of dice you would like to cast! (max 10):',
               xform=int,
               xform_err_msg='> Please enter a value between 1 and 10! ',
               validate=lambda val: val in range(1, 11) or '> Please enter a value between 1 and 10! ')

def roll_dice(dice):
    input('> Press Enter to cast dice!')
    sleep(1)
    return [randint(1, 6) for _ in range(dice)]

def cast():
    print('> Here comes cast number {}'.format(1 + len(casts)))
    casts.append(roll_dice(num_dice))
    print('> Here is your result: {}'.format(casts[-1]))

def statistics():
    for i, result in enumerate(casts):
        print('> *** Cast {} resulted in {}'.format(i + 1, result))

def finish():
    print('> The game finished after {} casts.'.format(len(casts)))
    print('> Good bye! ')
    input()
    exit()

actions = {
    'r': cast,
    's': statistics,
    'e': finish,
}

options_after_cast = """
> Options:
>    R - to cast again;
>    S - view stats;
>    E - exit the programme.
> """

options_after_stats = """
> Options:
>    R - to cast again;
>    E - exit the programme.
> """

choice = 'r'
while True:
    actions[choice]()
    choice = ask(options_after_stats if choice == 's' else options_after_cast,
                 xform=lambda val: val[0].lower(),
                 xform_err_msg='> Please make sure input is valid before pressing enter...',
                 validate=lambda val: val in actions or '> Ups! Available options R(new cast), S(statistics) si E(exit).')

Simplicity

Is it really necessary to suppress the "S" option after displaying statistics? Special cases like that prevent you from generalizing your logic, and add complexity to your program. If it weren't for that requirement, it might be possible to improve the code further, for example, by automatically generating the options menu text.

share|improve this answer
    
Thank you for your comment! Right before your post i realized the simplicity part as an important requirement to keep in mind :). Now studying your code proposal. –  user50916 Aug 11 at 8:25

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.