2
\$\begingroup\$

The program is a little code about guns with user input. The program is a perfect representation of the beauty of functions. What do you think of it?

import sys
import time

#determines reload time
def reloading(reload_time):
    print('\nReloading', end='')

    for dot in range(reload_time):
        time.sleep(1)
        print('.', end='')

#parameters including the numerator/denominator, fire rate, and reload speed
def gun(ROUNDS, left, fire_rate, the_reload):
    print('\nshoot!')
    #hold enter
    rounds = ROUNDS
    print('\n{}/{}'.format(rounds, left))

    while rounds > 0 or left > 0:
        shoot = input()
        rounds -= 1
        #decrement each time input is given
        print('{}/{}'.format(rounds, left))

        if rounds == 0 and left == 0:
            print('OUT OF AMMO')
            break
        elif shoot.upper() == 'DONE':
            sure = input('Are you sure?\n> ')
            if sure.lower() == 'y':
                start()
                #allocates back to gun selection 
        elif rounds == 0:
            if left <= ROUNDS:
                reloading(the_reload)
                rounds += left
                left -= left
                print('\n{}/{}'.format(rounds, left))
                #if bullets left is less than the magazine
            else:
                reloading(the_reload)
                rounds += ROUNDS
                left -= ROUNDS
                print('\n{}/{}'.format(rounds, left))

        time.sleep(fire_rate)
        #fire rate


def start():
    firearm = input('CHOOSE YOUR GUN (ENTER to exit)\n> ')

    if firearm.upper() == 'ASSAULT' or firearm.upper() == 'ASSUALT':
        gun(40, 130, 0.1, 7)
    elif firearm.upper() == 'SHOTGUN':
        gun(6, 30, 2, 12)
    elif firearm.upper() == 'SMG':
        gun(40, 200, 0, 5)
    elif firearm.upper() == 'SNIPER':
        gun(1, 15, 0, 5)
    elif firearm.upper() == 'MARKSMAN':
        gun(20, 100, 0.8, 9)
    elif firearm.upper() == 'PISTOL':
        gun(10, 100, 0.6, 3) 
    else:
        sys.exit()
start()
\$\endgroup\$

1 Answer 1

2
\$\begingroup\$

Apart from that I don't quite get the point of your game, there are a few things that caught my eye:


1. User input
Your user input system is not particularly intuitive to use. Without the source code the user has virtually no way to see what inputs are acceptable at the moment or what your game does.

2. Documentation
Your functions are documented like this at the moment:

#determines reload time
def reloading(reload_time):
    ...

Enter the official Style Guide for Python Code (aka PEP8)! There, the official recommendation on how to document function is to use """documentation strings""" like so:

def reloading(reload_time):
    """determines reload time"""
    ...

Documentation written in that style helps Python's built-in help(...) function as well as basically all Python IDEs to pick it up. Future-you, and depending on the path you choose also others, will thank you a lot if you learn this early and stick to it.

3. Start
The general notes on user input apply here. But apart from that, there is also a lot of repeated code here. You call firearm.upper() on every branch of the if statement, when a simple firearm = input('CHOOSE YOUR GUN (ENTER to exit)\n> ').upper() could do that in one place.

It's also common practice to wrap the code that is supposed to be run in a scripty-manner in if __name__ == "__main__":. This basically tells the Python interpreter (and also people looking at your code) that this part is meant to be run as script and is not part of the code that's eventually going to be imported into some other script. See also the official documentation or this Stack Overflow post for further details.

if __name__ == "__main__":
    # This makes sure you game will not start if you ever import the file
    start()

sys.exit() is also a bit uncommon here but we will touch on that in a moment.

4. Reloading
Slow reloading is quite a killer (pun intended). Especially if it looks like nothing is happening. When I did a test run with your game on my Windows laptop, nothing happened after Reloading appeared on the console until the "loading" actually finished. This was what I expected, since at least on Windows print does not automatically flush the buffer so you will see nothing in the reload cycle. Using print('.', end='', flush=True) fixes that.

5. Gun play gun(...) is were the "fun" starts. This is actually your main gameplay, so gun is likely not a really good name. Often methods have some kind of action description in their name like fire_gun(...) or something similar. Nouns like gun are usually only used for class names.

Since we are alread nit-picking about names, let's look at the paramters:

  • ROUNDS: all uppercase names are by convention usually reserved for global, odule, or class level constant values. Here, the capitalization seems to be used to disambiguate the value from the internal variable rounds. But why not choose a name that seems more appropriate for what the value actually represents like magazine_capacity?
  • left is very generic here and leaves room for interpretation. total_rounds or reserve_capacity sound like more appropriate names in this context here.
  • fire_rate is in its own right actually quite okay, although the variable name contradicts the way you use it. A rate is usually defined as something per amount of time, like shots per seconds. In your case it's actually used the other way round as time between two shots. So consider changing either of them to fit the other.
  • the_reload: Why this name? Why not time_to_reload?

Now that we have sorted this out, let's look at the control flow: On the top-most level you repeat the fire-till-empty-then-reload cycly until you are completely out of ammo. That's fine. There is a little bit of repeated code in rounds == 0 where print('\n{}/{}'.format(rounds, total_rounds)) could be used just once outside the inner if statement

elif rounds == 0:
    if total_rounds <= magazine_capacity:
        # if bullets left is less than the magazine
        ...
    else:
        ...
    print('\n{}/{}'.format(rounds, total_rounds))

The first case with if rounds == 0 and total_rounds == 0: is also fine. With elif shoot.upper() == 'DONE': things start to get a little bit troubling. The code in this block is as follows:

elif shoot.upper() == 'DONE':
    sure = input('Are you sure?\n> ')
    if sure.lower() == 'y':
        start()

Since this basically has nothing to do with the gameplay, the code breaks the single responsibility principle which is often recommended when thinking about/designing functions. This a higher level gameplay element and should better be placed in start(). Doing this will also help you to get rid of the yet to surface error caused by exceeding the maximum recursion depth. You can get at that point if you get into that branch of the if statement over and over again, calling start(), which then in turn calls gun(...), which then calls start() again, and so on and so forth, until the program crashes.

So how to do this in a better way?

def fire_gun(magazine_capacity, reserve_capacity, fire_rate, time_to_reload):
    """parameters including the numerator/denominator, fire rate, and reload speed"""

    ...

    while rounds > 0 or reserve_capacity > 0:
        shoot = input()

        rounds -= 1
        print('{}/{}'.format(rounds, reserve_capacity))

        if rounds == 0 and reserve_capacity == 0:
            print('OUT OF AMMO')
            break
        elif shoot.upper() == 'DONE':
            sure = input('Are you sure?\n> ')
            if sure.lower() == 'y':
                break
        elif rounds == 0:
            ...

        time.sleep(fire_rate)   # Faster!!


def start():
    while True:
        firearm = input('CHOOSE YOUR GUN (ENTER to exit)\n> ').upper()

        if firearm == 'ASSAULT':
            fire_gun(40, 130, 0.1, 7)
        ...
        else:
            break

In this version there is no recursion and you come back to the "weapons menu" whenever you are out of ammo of the user chooses to quit before. No recursion, no recursion limit, no crash! Cool.

6. Comments
You are using them wrong! Comments in source code are usually best used to explain why you are doing something, and not what you are doing, especially if it's quite obvious, like e.g.

rounds -= 1
# decrement each time input is given

This is clear from the code. No need to waste a comment on this.

Sometimes you have to do a little bit more to make a comment unnecessary, e.g. there at

if left <= ROUNDS:
    ...
    #if bullets left is less than the magazine

If you stick to the naming recommendations from above this can become

if reserve_capacity <= magazine_capacity:
    ...

See? No need for that comment any more! The code is now self-explaining.

On a not-strictly Python-related note: you are the first person I get to know who somehow likes to stick their comments below the line they are commenting on. Most people I know (including me) would expect the comment to live on the line above or on the same line as the code that comments belongs to.


Advanced ideas

You find that long if statement in start() cumbersome to read and hard to extend? You would like to extend your weapons locker with ease? You are willing to look into Python's magic box? Then read on.

Often this kind of long if statement can be transformed into something that's easier to maintain and extend using a Python dictionary. It a so called mapping, which maps a key to a value.

In your concrete example one could use something like this:

def start():
    guns = {
        "ASSAULT": lambda: fire_gun(40, 130, 0.1, 7),
        "SHOTGUN": lambda: fire_gun(6, 30, 2, 12),
        ...   # much more guns here
    }

    while True:
        firearm = input('CHOOSE YOUR GUN (ENTER to exit)\n> ').upper()

        try:
            guns[firearm]()
        except KeyError:
            break

The gun names are used as keys of the dictionary, this is quite obvious. Depending on your Python knowledge, you might also know something about lambda expressions as well. If not, think of them as functions or whatevere you like that can be "called", but without a explicit name. That's how guns[firearm]() works: For a given key, say "ASSAULT", the dictionary returns a lambda expression likelambda: fire_gun(40, 130, 0.1, 7)in our example. You could also assign this to a variable likeparameterized_version_of_fire_gun = guns[firearm]. You can think ofparameterized_version_of_fire_gunas a function at that moment that you would call like you callreloading`, for example. If it helps you can also think of it as a "normal" Python function:

def parameterized_version_of_fire_gun():
    fire_gun(40, 130, 0.1, 7)

So the next logical step is now to call parameterized_version_of_fire_gun() (← Note the opening and closing parenthesis).

As a bonus, this version would allow you to print guns.keys() to easily show the user all the possibilites to choose from.

\$\endgroup\$
1
  • \$\begingroup\$ UPDATED gist \$\endgroup\$
    – notak
    Jul 9, 2019 at 0:41

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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