10
\$\begingroup\$

I'm new to Python, and pretty much programming/scripting. I'm just looking for someone to critique my methods and tell me if there are more efficient ways to do things.

#!/usr/bin/env python3
#
#
# A quick script to create a civil war game


############################
# DECLARE GLOBAL VARIABLES #
############################


### Armies = [Inf,Cav,Art] ###

global compdefmult
global compattmult
global compchoice
global userdefmult
global userattmult
global userchoice
comparmy = [100,50,10]
userarmy = [100,50,10]


##################
# IMPORT MODULES #
##################


import os
from random import randint


##########################
# DEFINE GLOBAL FUNCTIONS #
###########################

def clearscreen():
    os.system('clear')


def newstart():
    clearscreen()
    print ('''

 CCCC  IIIII  V    V  IIIII  L      W   W   W  AA  RRRR
C        I     V  V     I    L       W  W  W  A  A R   R 
C        I      VV      I    L        W W W   AAAA RRRR
 CCCC  IIIII    VV    IIIII  LLLL     WW WW   A  A R   R

This is Civil War!

You will have Infantry, Calvary and Artillery to win the battles you need to turn the tide of the war.

Now, General, take your place in history!

''')

def printstatus():
    print ('Your total Infantry are:' , userarmy[0])
    print ('Your total Calvary are:' , userarmy[1])
    print ('Your total Artillery are:' , userarmy[2])
    print ('')
    print ('Computers total Infantry are:' , comparmy[0])
    print ('Computers total Calvary are:' , comparmy[1])
    print ('Computers total Artillery are:' , comparmy[2])
    print ('')
    mainmenu()


####################
# MAIN BATTLE CODE #    
####################

def compdigger():   
    global compchoice
    global compdefmult
    global compattmult
    compchoice = randint(1,3)
    if compchoice == 1:
        if comparmy[0] == 0:
            compdigger()
        else:
            compchoice = "Infantry"
            compdefmult = 3
            compattmult = 3
    elif compchoice == 2:
        if comparmy[1] == 0:
            compdigger()
        else:
            compchoice = "Calvary"
            compdefmult = 4
            compattmult = 2
    else:
        if comparmy[2] == 0:
            compdigger()
        else:
            compchoice = "Artillery"
            compdefmult = 2
            compattmult = 4
    return compchoice
    return compdefmult
    return compattmult

def takeaction():
    print ('Infantry have an offensive multiplier of 3 and a defensive multiplier of 3')
    print ('Calvary have an offensive multiplier of 2 and a defensive multiplier of 4')
    print ('Artillery have an offensive multiplier of 4 and a defensive multiplier of 2')
    print ('')
    print ('''

Do you want to use:

1) Infantry
2) Calvary
3) Artillery

    ''')
    print ("")
    action = input ("Choose one -> : ");
    if action == "1":
        userchoice = "Infantry"
        print ("")
        if userarmy[0] == 0:
            print ("You don't have any more infantry! Please choose another troop set!")
            takeaction()
        else:
            print ("MAAAAARRRRCH!!!!!")
            defmult = 3
            attmult = 3
            print ("")
    elif action == "2":
        userchoice = "Calvary"
        print ("")
        if userarmy[1] == 0:
            print ("You don't have any more calvary! Please choose another troop set!")
            takeaction()
        else:
            print ("CHAAAAARRRGE!!!!!!")
            defmult = 4
            attmult = 2
            print ("")
    elif action == "3":
        userchoice = "Artillery"
        print ("")
        if userarmy[2] == 0:
            print ("You don't have any more artillery! Please choose another troop set!")
            takeaction()
        else:
            print ("FIIIIIIIRE!!!!!!")
            defmult = 2
            attmult = 4
            print ("")
    else:
        print ("")
        print ("You did not choose 1,2 or 3. Please choose a valid answer")
        print ("")
        takeaction()

    print ("")

    userdefroll = randint(1,12)
    userattroll = randint(1,12)

    print ("Your defensive roll was ", userdefroll)
    print ("Your attack roll was " , userattroll)

    compdefroll = randint(1,12)
    compattroll = randint(1,12)

    print ("")

    print ("Computers defensive roll was " , compdefroll)
    print ("Computers attack roll was " , compattroll)


    print ("")

    userattack = int(attmult) * int(userattroll)
    userdefense = int(defmult) * int(userdefroll)

    print ("")
    print ("Defense total is multiplier %s times roll %s which has a total of %s " % (defmult, userdefroll , userdefense)) 
    print ("Attack total is miltiplier %s times roll %s which has a total of %s " % (attmult, userattroll, userattack))
    print ("")

    compdigger()

    compdefense = int(compdefmult) * int(compdefroll)
    compattack = int(compattmult) * int(compattroll)

    print ("")
    print ("The computer chose ", compchoice)
    print ("")
    print ("Computers total Defense multiplier is %s times it's roll %s which has a total of %s " % (compdefmult, compdefroll, compdefense))
    print ("Computers total Attack multiplier is %s times it's roll %s which has a total of %s " % (compattmult, compattroll, compattack))
    print ("")

    print ("")
    if compdefense >= userattack:
        print ("The computer defended your attack without troop loss")
    else:
        userdiff = int(userattack) - int(compdefense)
        print ("Computer lost %s  %s " % (userdiff , compchoice))
        if compchoice == "Infantry":
            comparmy[0] -= int(userdiff)
            if comparmy[0] < 0:
                comparmy[0] = 0
        elif compchoice == "Calvary":
            comparmy[1] -= int(userdiff)
            if comparmy[1] < 0:
                comparmy[1] = 0
        else:
            comparmy[2] -= int(userdiff)
            if comparmy[2] < 0:
                comparmy[2] = 0 

    if userdefense >= compattack:
        print ("You defended the computers attack without troop loss")
    else:
        compdiff = int(compattack) - int(userdefense)
        print ("You lost %s  %s " % (compdiff , userchoice))
        if userchoice == "Infantry":
            userarmy[0] -= int(compdiff)
            if userarmy[0] < 0:
            userarmy[0] = 0
        elif userchoice == "Calvary":
            userarmy[1] -= int(compdiff)
            if userarmy[1] < 0:
                userarmy[1] = 0
        else:
            userarmy[2] -= int(compdiff)
            if userarmy[2] < 0:
                userarmy[2] = 0

    if userarmy[0] == 0 and userarmy[1] == 0 and userarmy[2] == 0:
        print ("Your army has been decemated! RETREAT!")
        exit(0)

    if comparmy[0] == 0 and comparmy[1] == 0 and comparmy[2] ==0:
        print ("The computers army has been decemated! VICTORY!")
        exit(0)

    print ("")


    mainmenu()


def mainmenu():
    print ('''

MAIN MENU
---------

1) Print out army status
2) ATTACK!
3) Save game (not implemented yet)
4) Load game (not implemented yet)
5) Quit

''')

    menuaction = input ("Please choose an option -> : ");
    if menuaction == "1":
        printstatus()
    elif menuaction == "2":
        takeaction()
    elif menuaction == "3":
        print ("Not implemented yet!")
        mainmenu()
    elif menuaction == "4":
        print ("Not implemented yet!")
        mainmenu()
    elif menuaction == "5":
        print ("Good bye!")
        exit (0)
    else:
        print ("You did not choose 1,2,3 or 4. Please choose a valid option!")
        mainmenu()

newstart()
mainmenu()
\$\endgroup\$

migrated from stackoverflow.com May 23 '15 at 23:13

This question came from our site for professional and enthusiast programmers.

  • 2
    \$\begingroup\$ A few quick notes: If you want to loop forever, use while True:, not a function that calls itself recursively. You can use random.choice(['Infantry', 'Cavalry', 'Artillery']) instead of using randint(1, 3) and then comparing to 1, 2, and 3. And you can use a dict mapping inputs to functions instead of a long if-elif chain that just does the same thing manually. \$\endgroup\$ – abarnert May 23 '15 at 23:07
  • 2
    \$\begingroup\$ One more thing: to return three numbers, do return compchoice, compdefmult, compattmult. If you do three separate return statements, only the first one gets executed. Also, the caller should actually store those returned values somewhere and use them. And if you do that, you can get rid of all of the globals. \$\endgroup\$ – abarnert May 23 '15 at 23:32
  • \$\begingroup\$ I'm not sure what you mean by >"the caller should actually store those returned values somewhere and use them" I've always wondered what the solution around getting rid of globals would be. Can you elaborate a bit? Thanks! Chris \$\endgroup\$ – Chris Gleason May 24 '15 at 6:46
  • \$\begingroup\$ Also about the returns, so currently as written, only the value for compchoice is being returned? \$\endgroup\$ – Chris Gleason May 24 '15 at 6:47
  • \$\begingroup\$ Exactly, only the compchoice is being returned. \$\endgroup\$ – abarnert May 24 '15 at 7:12
10
\$\begingroup\$

As mentioned by @abarnert, you're using infinite recursion to "repeat forever". By default, after you've recursed more than 1000 times, your program will throw an error. The better way to do this would be to use a while condition loop. In this case, your takeaction function would become this:

def take_action() -> None:
    """
    Add a description of take_action here.
    """
    while True:
        ...

You can also use a dictionary of functions containing actions rather than chaining if condition statements. Usually it's looks better. Here's a basic example of what that could look like:

def my_action() -> None:
    """
    Description
    """
    ...

actions = {
    "action": my_action
}

user_input = raw_input()
if user_input in actions:
    actions[user_input]()

I'd also recommend using docstrings to document how your code works. They're very important, because they describe how your code works. Without them, code can be understandable, but can be very hard to understand. Here's an example:

def my_function(args) -> None:
    """
    Add a description of the function here.
    """
    ...

Your clearscreen() function isn't very portable, and will only work on linux-based systems. I'd recommend using something like this instead:

def clear_screen():
    os.system("cls" if os.name == "nt" else "clear")

Finally, I'd recommend reading PEP8, Python's official style guide. It essentially tells you how your code should be formatted, and how it should look. If you want to check your code for PEP8 issues, you can use this online checker.

\$\endgroup\$
  • \$\begingroup\$ Thanks for the feedback! If I used a dictionary that contained actions, wouldn't I still need to use an if statement to call those? Do you have a link to an example so I can take a look? Also is the 1000 recurses something that's configurable? Just wondering? I'll replace with the while do loop now. I'll run through the style checker, but was there something specific you saw that was a big no no? Thanks! Chris \$\endgroup\$ – Chris Gleason May 24 '15 at 6:44
  • 1
    \$\begingroup\$ @ChrisGleason: No, you don't need an if statement for each action if you have a dictionary; that's the whole point. You either need a single if statement, like Ethan showed here, or no if statements at all if you call actions.get(user_input, handle_bad_input). \$\endgroup\$ – abarnert May 24 '15 at 7:11
  • 2
    \$\begingroup\$ @ChrisGleason: Meanwhile, the most obvious PEP 8 violation is putting spaces between the function name and the ( in most of your function calls. (And the fact that it's most, but not all, makes it inconsistent, which is an even bigger no-no.) \$\endgroup\$ – abarnert May 24 '15 at 7:19
  • 1
    \$\begingroup\$ @ChrisGleason Technically, yes, the recursion is "configurable", but it then can only be set to a maximum of 2^31 - 1. While that's a lot, it's still a bad design. \$\endgroup\$ – Ethan Bierlein May 24 '15 at 15:03
  • 1
    \$\begingroup\$ I just noticed that you're implicitly suggesting using PEP 484 type hints. Do you think that's worth recommending for code reviews on Python 3 code? (It's not a bad idea, since they're harmless in 3.0-3.4 and may be helpful in 3.5+, I'm just worried about having to explain unfamiliar features that to novices when they can't just look them up in the docs.) \$\endgroup\$ – abarnert May 24 '15 at 17:59
7
\$\begingroup\$

The biggest problem in this code is using recursive function calls instead of looping, as I explained in my comment and Ethan Bierlein's answer explains in a lot more detail. (Note that you're doing this in more than one place—e.g., compdigger calls itself instead of looping too. You'll want to fix all of them, not just one.)

The second biggest problem is the way you're passing and returning values to and from your functions.

At the end of compdigger, you do this:

return compchoice
return compdefmult
return compattmult

The first return statement exits the function, returning the compchoice value. The other statements never happen.

If you want to return three values, put them all in one return statement:

return compchoice, compdefmult, compattmult

(Technically, this is still only returning a single value, which just happens to be a tuple whose three members are the three values you wanted to return. But you can ignore that detail for now.)

Meanwhile, a return doesn't do any good unless someone actually uses the returned value. But you never do; you just call compdigger(). If you're going through all the trouble to return three values, you should assign them to three variables:

compchoice, compdefmult, compattmult = compdigger()

And once you do that, there's no reason for your global statements. Instead of modifying three global variables, compdigger is just returning 3 values; instead of accessing the global variables, takeaction is just accessing its own local variables, which it set from the result of calling compdigger().

The comparmy variable is being used, but not modified, within compdigger, so you can pass that as arguments. Change the definition to:

def compdigger(comparmy):

… and the call to:

compchoice, compdefmult, compattmult = compdigger(comparmy)

You still have four more globals—userchoice, userdefmult, userattmult, and userarmy—but those are only being used at all within takeaction, so there's no reason for them to be globals in the first place.

And, once you've removed all of those, you have no non-constant global variables left, which is what you're aiming for.


My next suggestion was to use a dict as a function lookup table instead of a big elif chain; Ethan Bierlein's answer again covers that in a lot more detail.


Next, if you have three values to choose from, instead of this:

if i == 1:
    i = 'Infantry'
elif i == 2:
    i = 'Cavalry'
elif i == 3:
    i = 'Artillery'

… you can use a dict again, like this:

forces = {1: 'Infantry', 2: 'Cavalry', 3: 'Artillery'}
i = forces[i]

And if you share that forces dict between the user-turn and computer-turn code, your computer code just does:

i = forces[random.randint(1, 3)]

… or, maybe even better:

i = random.choice(forces.values())

At the top of takeaction, you're mixing a bunch of separate one-line print calls and one multi-line print call. There might sometimes be a good reason for that, but I don't see why here you're not just using one big multi-line call. And if you did that, you could fit your source code into 80 columns, which makes it more readable for people who want to read it on such obsolete places as default-sized terminal windows, emacs frames, or StackOverflow questions.


os.system('clear') isn't portable. On Windows, the corresponding command is called cls, not clear. And even on non-Windows systems, it's not guaranteed by POSIX, it just happens to be part of the GNU and BSD userland—which is fine if you care only about, say, Mac, Linux, and FreeBSD, but not if you want it to work on all Unix systems.

There is no actual portable way to do what you want (short of printing a bunch of blank lines and assuming that's probably good enough), and that may be fine, just be aware that you're limiting your portability.


If you want to push your learning a little bit farther, this game would make a great way for you to learn about classes. You have some clearly-definable objects here: the Game object, two Player objects, one of which is an AIPlayer subclass, and maybe even Unit objects with three subclasses (although I personally don't think I'd model the units that way). This is obviously a pretty big change, and it's definitely not necessary to make your code good; it's just a different way to think of the same program, and you would learn a lot by trying to redesign it that way.


Meanwhile, this isn't really a comment on your code, but on your question: asking for a "more efficient" way to do something is usually not a useful question.

"Efficient" can mean different things—less CPU time, less total time, less heat generated, less memory or disk space used, or even less development time. These are often tradeoffs—e.g., you can make it faster by using more memory—so just "more efficient" is in general impossible to answer.

More importantly, it's usually not relevant. For example, if your game is running more fast enough, never going over 10% CPU usage, and only using 46K of memory, how would it benefit you to make it faster or less CPU-hungry or smaller? Making your code less readable to make it more efficient is a bad tradeoff when it's already efficient enough. And, even when your code isn't efficient enough, it's almost always some particular bottleneck that's inefficient, and that's the only part you want to optimize; everything else, you're better off with readable code than efficient code.

Of course there are some cases where code is just algorithmically bad (e.g., if you're doing a quadratic search where you could have done an instant lookup), but usually in those cases, the more efficient code is also more readable and idiomatic anyway.


Finally, while this isn't a problem in your game logic, two issues with your text.

First, some of your output is wider than 80 columns. Most people run command-line programs in an 80-column terminal window, so for example, your game banner will look something like:

You will have Infantry, Calvary and Artillery to win the battles you need to turn t
he tide of the war.

Second, you've misspelled "cavalry" as "calvary", multiple times. The first one means soldiers on horses with sabres; the second one means a representation of Jesus's crucifixion. A civil war grognard will sneer at your game for that. :)

\$\endgroup\$
  • \$\begingroup\$ lol. Thanks. Haven't had time to read through all of this yet, but i will tonight. Thanks for all the feedback! \$\endgroup\$ – Chris Gleason May 24 '15 at 8:42
  • 1
    \$\begingroup\$ os.system("cls" if os.name == "nt" else "clear") is the most portable way to clear the screen atm. \$\endgroup\$ – Ethan Bierlein May 24 '15 at 15:01
  • 2
    \$\begingroup\$ @EthanBierlein More portable than os.system('clear'), sure, but hardly "the most portable". That will still fail or do the wrong thing on systems with a minimal userland, or when the program is run inside IDLE, etc. A more portable thing would probably be to check isatty, then check for termcaps (or maybe try curses first), then fall back to the ANSI/VT100 sequence or the clear command only if there's no termcap lib. What matters is what's "portable enough" for the intended uses. (If he only cares about running interactively in Terminal.app on his Mac, he's already done.) \$\endgroup\$ – abarnert May 24 '15 at 16:24
  • \$\begingroup\$ Since you're both commenting on this I'll comment here since you'll both be notified. I was finally getting back to this and came across this link when looking into formatting while loops. What do you think about them saying while loops are significantly slower? I think in this particular case, it's probably negligent, and I'm sure nesting recursive functions probably is slow too but wanted to see what yo thought of it. Still reformatting my code and trying to figure out how to rip my functions out of recursion and put them into a while loop. \$\endgroup\$ – Chris Gleason Jun 20 '15 at 21:38
  • \$\begingroup\$ @ChrisGleason The problem with recursive function calls isn't about it being fast or slow - the problem is, using a recursive call to loop "forever" will cause your program to eventually crash if someone plays your game a long time. The 'while' loop doesn't have that problem. The page you linked to is talking about the relative speed of 'while True' vs. 'while 1', which is probably negligent in any case. I would use 'while True' because it seems to be more common in Python code. \$\endgroup\$ – Brandin Feb 1 '16 at 14:05

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.