My first Python Program, it's too messy

[AJG]

My first attempt at programming using Python was to make a calculator for an online game I play called AstroEmpires.

The user can input the number of bases they have, the shipyards on each base and the base's production value. They then tell the program which ship they wish to build and how many of that ship. The calculator tells them how much this will cost, how many of the ship to build at each base for maximum efficiency and how long this will take. It also saves the base, shipyard and production values.

It works, but I think that it is too messy; it takes 206 lines of code. I am wondering if there is anything I can do to condense the code.

Any help will be appreciated :)

loop = 1

ships = ['	Fighters	'	,\
'	Bombers	'	,\
'	Heavy Bombers	'	,\
'	Ion Bombers	'	,\
'	Corvette	'	,\
'	Recycler	'	,\
'	Destroyer	'	,\
'	Frigate	'	,\
'	Ion Frigate	'	,\
'	Scout Ship	'	,\
'	Outpost Ship	'	,\
'	Cruiser	'	,\
'	Carrier	'	,\
'	Heavy Cruiser	'	,\
'	Battleship	','Fleet Carrier']
ships_cost = {'	Fighters	'	:	5	,	\
'	Bombers	'	:	10	,	\
'	Heavy Bombers	'	:	30	,	\
'	Ion Bombers	'	:	60	,	\
'	Corvette	'	:	20	,	\
'	Recycler	'	:	30	,	\
'	Destroyer	'	:	40	,	\
'	Frigate	'	:	80	,	\
'	Ion Frigate	'	:	120	,	\
'	Scout Ship	'	:	40	,	\
'	Outpost Ship	'	:	100	,	\
'	Cruiser	'	:	200	,	\
'	Carrier	'	:	400	,	\
'	Heavy Cruiser	'	:	500	,	\
'	Battleship	'	:	2000	,	\
'	Fleet Carrier	'	:	2500		\
}

ships_shipyard = {'	Fighters	'	:	1	,	\
'	Bombers	'	:	2	,	\
'	Heavy Bombers	'	:	3	,	\
'	Ion Bombers	'	:	3	,	\
'	Corvette	'	:	4	,	\
'	Recycler	'	:	5	,	\
'	Destroyer	'	:	6	,	\
'	Frigate	'	:	8	,	\
'	Ion Frigate	'	:	8	,	\
'	Scout Ship	'	:	4	,	\
'	Outpost Ship	'	:	8	,	\
'	Cruiser	'	:	10	,	\
'	Carrier	'	:	12	,	\
'	Heavy Cruiser	'	:	12	,	\
'	Battleship	'	:	16	,	\
'Fleet Carrier'	:	16	}

print "Welcome to AE production calculator, written by Angus Goldsmith, a.k.a. Cowbacca!"
while loop == 1:
    print"1) Yes"
    print"2) No"
    saved = input("Use saved data?")
    if saved == 1:
        import pickle
        unpickleshipyards = open('shipyardlevels.txt', 'r')

        # now load the list that we pickled into a new object
        unpickledshipyards = pickle.load(unpickleshipyards)
    
        # close the file, just for safety
        unpickleshipyards.close()
        base_shipyards = unpickledshipyards
    
    
        import pickle
        unpickleproduction = open('baseproduction.txt', 'r')
    
        # now load the list that we pickled into a new object
        unpickledproduction = pickle.load(unpickleproduction)
    
        # close the file, just for safety
        unpickleproduction.close()
        base_production = unpickledproduction   
    
        import pickle
        unpicklebases = open('bases.txt', 'r')
    
        # now load the list that we pickled into a new object
        unpickledbases = pickle.load(unpicklebases)
    
        # close the file, just for safety
        unpicklebases.close()
        bases = unpickledbases
    else:
        bases = 0
        base_production = []
        base_shipyards = []


    if saved == 2:
        bases = input("Type your number of bases:")
        import pickle
        file = open('bases.txt', 'w')
        pickle.dump(bases,file)
        file.close()

    if saved == 2:
        temp_bases = bases

        shipyards = 0
        base_number1 = 1
        while temp_bases > 0:
            print "Type Base ",base_number1,"'s shipyard level:"
            base_shipyards.append (input())
            base_number1 = base_number1+1
            temp_bases = temp_bases-1
            print ""
        temp_bases2 = bases

        import pickle
        file = open('shipyardlevels.txt', 'w')
        pickle.dump(base_shipyards,file)
        file.close()
    base_number = 1
    if saved == 2:    
        while temp_bases2 > 0:

            print "Type Base ",base_number,"'s production value:"
            base_production.append (input())
            temp_bases2 = temp_bases2-1
            base_number = base_number+1
            print ""
        import pickle
        file = open('baseproduction.txt', 'w')
        pickle.dump(base_production,file)
        file.close()
    choice = 0
    print "Ship list:"
    print "	1)	Fighters"
    print "	2)	Bombers	"
    print "	3)	Heavy Bombers	"
    print "	4)	Ion Bombers	"
    print "	5)	Corvette	"
    print "	6)	Recycler	"
    print "	7)	Destroyer	"
    print "	8)	Frigate	"
    print "	9)	Ion Frigate	"
    print "	10)	Scout Ship	"
    print "	11)	Outpost Ship	"
    print "	12)	Cruiser	"
    print "	13)	Carrier	"
    print "	14)	Heavy Cruiser	"
    print "	15)	Battleship	"
    print "	16)	Fleet Carrier	"

    
    choice = input("What do you wish to produce?")-1
    
    quantity = 0
    
    print "How many",ships[choice],"do you want to build?"
    
    quantity = input("Quantity:")
    
    ship_name = ships[choice]
    
    total_cost = ships_cost[ship_name]*quantity
    
    print "Total cost is ",total_cost,"."
    
    temp_bases3 = 0
    
    temp_bases3 = bases
    
    total_production = 0



        
    
    while temp_bases3 >0:
        base_prod = base_production[temp_bases3-1]
        total_production = total_production + base_prod
        temp_bases3 = temp_bases3-1
    
    temp_bases4 = bases
    
    while temp_bases4 >0:
        if base_shipyards[temp_bases4-1]<ships_shipyard[ship_name]:
            total_production = total_production-base_production[temp_bases4-1]
        temp_bases4 = temp_bases4-1
    
    temp_bases5 = bases
    
    while temp_bases5 >0:
        if base_shipyards[temp_bases5-1]>=ships_shipyard[ship_name]:
            production_percentage = (base_production[temp_bases5-1]*1.0)/total_production
            print "Build ",((total_cost*production_percentage)/ships_cost[ship_name]),ship_name,"at base ", temp_bases5
            print "This will take",((total_cost*production_percentage)/(base_production[temp_bases5-1]*1.0))," hours, or ",((total_cost*production_percentage)/(base_production[temp_bases5-1]*1.0))*60," minutes."
            print ""
        else:
            print "Base ",temp_bases5,"does not have suffecient shipyards."
        temp_bases5 = temp_bases5-1
    print ""
    print ""
    print ""


    exit_choice = input("Press 1 to exit or 2 to do another calculation.")
    if exit_choice == 1:
          loop = 0

[finite4dw]

Hi AJG,

Here are a few suggestions:

In the section where you are loading saved data, consider making a function to load the data. That should cut down the code in that section to a third.

Say, you called this function "load_saved", then your loading section would look like this:

if saved == 1:
    base_shipyards = load_saved('baseshipyardlevels.txt')
    base_production = load_saved('baseproduction.txt')
    bases = load_saved('bases.txt')

Now, let us look at the function. Your original code that should go into the function looks like this:

import pickle
unpickleshipyards = open('shipyardlevels.txt', 'r')

# now load the list that we pickled into a new object
unpickledshipyards = pickle.load(unpickleshipyards)
    
# close the file, just for safety
unpickleshipyards.close()
base_shipyards = unpickledshipyards

The following will be the function based on the above code. Note any differences:

def load_saved_data(file_name):
    import pickle
    
    data_file = open(file_name, 'r')

    # now load the list that we pickled into a new object
    saved_data = pickle.load(data_file)
    
    # close the file, just for safety
    data_file.close()
    
    return saved_data

Note that the only changes are:

1. I have used more generic names for the variables because we are no longer dealing with just bases or shipyards, but anything that is to be persisted to storage (pickled, in this case).

2. There is a return statement. This is needed to return a particular piece of data (in our case, the saved_data).

You can try to find other sections where you can move repeated code into functions. I will post a couple more replies with more suggestions soon.

Hope this helps,

Jayce

My first attempt at programming using Python was to make a calculator for an online game I play called AstroEmpires.

The....

It works, but I think that it is too messy; it takes 206 lines of code. I am wondering if there is anything I can do to condense the code.

Any help will be appreciated :)

...

[AJG]

Ah, yes, that helps, thank you.

I can do the same thing with the code for saving data too.

[AJG]

Update:
I have made a save data function:

def save_data(file_name,data):
        import pickle
        data_file = open(file_name, 'w')
        pickle.dump(data,data_file)
        file.close()

save_data('shipyardlevels.txt',base_shipyards)

Have I made this correctly?

[finite4dw]

Hi AJG,

Here are a couple more suggestions:

1. When you are coding in a list or dictionary literal ([ "one", "two" ]) you don't need to use the line continuation backslashes. Also, it is perfectly OK to leave a trailing comma (some would even recommend leaving the trailing comma in there).

e.g.:

a_list = [
    "one",
    "two",
    "three",
]

a_dict = {
    "one" : "uno",
    "two" : "due",
   "three" : "tre",
}

2. Use the list of ships to print the ship list. This has two advantages:

• you don't need to type in the whole list over again
• you won't make a mistake in the order etc.

Consider this function that generates the list of ships:

def print_ship_list(ships):
    """Prints the list of ships.
    """
    print "Ship list:"
    ship_no = 1
    for ship in ships:
        print "	 %2d)    %s" % (ship_no,  ship)
        ship_no += 1

Now, all you need to do is call this function with the list of ships:

choice = 0
   print_ship_list(ships)
   choice = input("What do you wish to produce?") - 1

In general, if you need to generate a printout of some existing list, especially if the order is important, always use that list to generate the output. This will avoid pointless typing and potential errors.

Also, try not put the formatting in the input data. Just put plain data, and then use the program to format it as needed. This way keys will be safer (because keys need to be exact and it can be very difficult to count the number of spaces).

Also, when you need to format the same thing in two different way, you can just use it---no need to strip the existing formatting.

Hope this was helpful,

Jayce

My first attempt at programming using Python was to make a calculator for an online game I play called AstroEmpires.
...
It works, but I think that it is too messy; it takes 206 lines of code. I am wondering if there is anything I can do to condense the code.

Any help will be appreciated :)

ships = ['	Fighters	'	,\
'	Bombers	'	,\
'	Heavy Bombers	'	,\
'	Ion Bombers	'	,\
# ...
'	Heavy Cruiser	'	,\
'	Battleship	','Fleet Carrier']

#...

    choice = 0
    print "Ship list:"
    print "	1)	Fighters"
    print "	2)	Bombers	"
    print "	3)	Heavy Bombers	"
    print "	4)	Ion Bombers	"
# ...
    print "	14)	Heavy Cruiser	"
    print "	15)	Battleship	"
    print "	16)	Fleet Carrier	"

    
    choice = input("What do you wish to produce?")-1
    
   #...

[AJG]

Okay, that all makes sense, apart from this bit:

print " %2d) %s" % (ship_no, ship)
ship_no += 1

I do not understand what this code tells the computer to do, would you care to enlighten me?

[finite4dw]

Hi AJG,

The code looks correct.

I would also recommend that you use the binary flag flag ('b') when opening files for pickling (both loading and saving).

# opening for reading
 open(file_name, 'rb')

# opening for writing
 open(file_name, 'wb')

This is especially an issue on Windows (and probably VMS and OS/2 too) where "text files" are special to the OS and could lead to corrupted data. For the sake of portability, include the binary flag, even if you are on a non-Windows machine.

Thanks,

Jayce

Update:
I have made a save data function:

def save_data(file_name,data):
        import pickle
        data_file = open(file_name, 'w')
        pickle.dump(data,data_file)
        file.close()

save_data('shipyardlevels.txt',base_shipyards)

Have I made this correctly?

[AJG]

Thank you, I am installing Ubuntu on another partition soon, so I guess that the binary tags will help my program run on that without alterations.

[finite4dw]

Hi AJG,

print " %2d) %s" % (ship_no, ship)
ship_no += 1

This is roughly equivalent to the following:

print "  " + ship_no + ") " + ship
ship_no = ship_no + 1

Note that the second line is a short-cut for incrementing by the given value. If you have programmed in any of C/C++/Java/C#/Perl, you should be familiar with this, so I apologize if I mentioned something you already knew.

The first line would seem really weird if you haven't been subjected to printf() operations (It is available in C/C++/Perl and in the latest version of Java (5 and above); not sure about C#)).

The first piece you give to a formatting function is the format string. This describes how your string should be formatted. Then you give it all the data that you wish to use inside the format string in the order in which their markers appear in the format string.

In our example we have two markers "%2d" and "%s". The first will be substituted with the first value we provided, and the second one with the second. All the other pieces of the format string

# format string  formatting operator  (data values)
" %2d) %s "          %                (ship_no, ship)

Just remember that %d should be given a number and %s a string; otherwise you will get type error (this is because Python is strongly typed). If you are concerned that a value could be non-string just wrap it with an str().

The advantage of using format strings is that you can concisely describe how your data should be formatted.

For instance, in the example above, we specified that the ship number should be formatted with a leading space if the number of characters is less than 2 (1 will be formatted as " 1" and "11" as "11" and everything will line-up nicely). If we were to manually perform this formatting, the code will be quite messy.

For the complete set of string formatting operation symbols, see "String Formatting Operations" section of the standard python library reference (this can be found at the following URL:)

http://docs.python.org/lib/typesseq-strings.html

If this string formatting business sounds too weird to you, just don't use it. Also, the formatting scheme is changing in Python 3.0. So, maybe I made a mistake introducing it to you if you were untouched by this weirdness :-)

Remember, that you should not shy away from using this if you need to.

Thanks,

Jayce

Okay, that all makes sense, apart from this bit:

I do not understand what this code tells the computer to do, would you care to enlighten me?

[AJG]

Thank you, that clears it up for me, I can see how that would be useful.

Thank you again for all your help! :)

[AJG]

I have a problem... When I run it inside PythonWin, my program works fine. However, when I run it outside of PythonWin, it crashes after the user inputs their number of bases. Here is the code:

loop = 1

def print_ship_list(ships):
    """Prints the list of ships.
    """
    print "Ship list:"
    ship_no = 1
    for ship in ships:
        print "	 %2d)    %s" % (ship_no,  ship)
        ship_no += 1

def load_saved(file_name):
    import pickle
    
    data_file = open(file_name, 'rb')

    # now load the list that we pickled into a new object
    saved_data = pickle.load(data_file)
    
    # close the file, just for safety
    data_file.close()
    
    return saved_data

def save_data(file_name,data):
        import pickle
        data_file = open(file_name, 'wb')
        pickle.dump(data,data_file)
        file.close()

ships = ['	Fighters	'	,
'	Bombers	'	,
'	Heavy Bombers	'	,
'	Ion Bombers	'	,
'	Corvette	'	,
'	Recycler	'	,
'	Destroyer	'	,
'	Frigate	'	,
'	Ion Frigate	'	,
'	Scout Ship	'	,
'	Outpost Ship	'	,
'	Cruiser	'	,
'	Carrier	'	,
'	Heavy Cruiser	'	,
'	Battleship	','Fleet Carrier']
ships_cost = {'	Fighters	'	:	5	,	\
'	Bombers	'	:	10	,	\
'	Heavy Bombers	'	:	30	,	\
'	Ion Bombers	'	:	60	,	\
'	Corvette	'	:	20	,	\
'	Recycler	'	:	30	,	\
'	Destroyer	'	:	40	,	\
'	Frigate	'	:	80	,	\
'	Ion Frigate	'	:	120	,	\
'	Scout Ship	'	:	40	,	\
'	Outpost Ship	'	:	100	,	\
'	Cruiser	'	:	200	,	\
'	Carrier	'	:	400	,	\
'	Heavy Cruiser	'	:	500	,	\
'	Battleship	'	:	2000	,	\
'	Fleet Carrier	'	:	2500		\
}

ships_shipyard = {'	Fighters	'	:	1	,	
'	Bombers	'	:	2	,	
'	Heavy Bombers	'	:	3	,	
'	Ion Bombers	'	:	3	,	
'	Corvette	'	:	4	,	
'	Recycler	'	:	5	,	
'	Destroyer	'	:	6,	
'	Frigate	'	:	8	,	
'	Ion Frigate	'	:	8	,	
'	Scout Ship	'	:	4	,	
'	Outpost Ship	'	:	8	,	
'	Cruiser	'	:	10	,	
'	Carrier	'	:	12	,	
'	Heavy Cruiser	'	:	12	,	
'	Battleship	'	:	16	,	
'Fleet Carrier'	:	16,	}

print "Welcome to AE production calculator, written by Angus Goldsmith, a.k.a. Cowbacca!"
while loop == 1:
    print"1) Yes"
    print"2) No"
    saved = input("Use saved data?")

    if saved == 1:
        base_shipyards = load_saved('shipyardlevels.txt')
        base_production = load_saved('baseproduction.txt')
        bases = load_saved('bases.txt')
    
    else:
        bases = 0
        base_production = []
        base_shipyards = []


    if saved == 2:
        bases = input("Type your number of bases:")
        save_data('bases.txt',bases)
# PROGRAM CRASHES HERE

        temp_bases = bases
        
        shipyards = 0
        base_number1 = 1
        while temp_bases > 0:
            print "Type Base ",base_number1,"'s shipyard level:"
            base_shipyards.append (input())
            base_number1 = base_number1+1
            temp_bases = temp_bases-1
            print ""

        save_data('shipyardlevels.txt',base_shipyards)
        
        base_number = 1
        temp_bases2 = bases
        while temp_bases2 > 0:

            print "Type Base ",base_number,"'s production value:"
            base_production.append (input())
            temp_bases2 = temp_bases2-1
            base_number = base_number+1
            print ""
        save_data('baseproduction.txt',base_production)
            
    choice = 0
    print_ship_list(ships)

    
    choice = input("What do you wish to produce?")
    
    quantity = 0
    
    print "How many",ships[choice-1],"do you want to build?"
    
    quantity = input("Quantity:")
    
    ship_name = ships[choice]
    
    total_cost = ships_cost[ship_name]*quantity
    
    print "Total cost is ",total_cost,"."
    
    temp_bases3 = 0
    
    temp_bases3 = bases
    
    total_production = 0



        
    
    while temp_bases3 >0:
        base_prod = base_production[temp_bases3-1]
        total_production = total_production + base_prod
        temp_bases3 = temp_bases3-1
    
    temp_bases4 = bases
    
    while temp_bases4 >0:
        if base_shipyards[temp_bases4-1]<ships_shipyard[ship_name]:
            total_production = total_production-base_production[temp_bases4-1]
        temp_bases4 = temp_bases4-1
    
    temp_bases5 = bases
    
    while temp_bases5 >0:
        if base_shipyards[temp_bases5-1]>=ships_shipyard[ship_name]:
            production_percentage = (base_production[temp_bases5-1]*1.0)/total_production
            print "Build ",((total_cost*production_percentage)/ships_cost[ship_name]),ship_name,"at base ", temp_bases5
            print "This will take",((total_cost*production_percentage)/(base_production[temp_bases5-1]*1.0))," hours, or ",((total_cost*production_percentage)/(base_production[temp_bases5-1]*1.0))*60," minutes."
            print ""
        else:
            print "Base ",temp_bases5,"does not have suffecient shipyards."
        temp_bases5 = temp_bases5-1
    print ""
    print ""
    print ""


    exit_choice = input("Press 1 to exit or 2 to do another calculation.")
    if exit_choice == 1:
          loop = 0

Does anybody know exactly what is going wrong?

[woooee]

When the users makes a choice, i.e.
choice = input("What do you wish to produce?")-1
you should test for bounds. (choice < 1) or (choice > max) would yield an error and ask the user to choose again.

[AJG]

I don't see how that would help, it worked before without testing for bounds, unless I am misunderstanding how that would work?

[woooee]

def save_data(file_name,data):

if saved == 2:
bases = input("Type your number of bases:")
save_data('bases.txt',bases) ## bases is just a number and not data isn't it?
# PROGRAM CRASHES HERE

What happens when someone keys in an "a" instead of a number, or 155 instead of 15?

[AJG]

If they type "a" it crashes. If they type anything it crashes.

When it worked, 155 would be fine, it is possible (though unlikely) to have 155 bases.

[woooee]

Let's forget about bounds checking for now. I'm talking about a different section of the program. What does "crashes" mean. What is the error message and does it actually save the data (look at the length and time stamp for the file).

Edit: I think this is the problem

def save_data(file_name,data):
   import pickle        ## should be at the beginning of the program so it imports once 
   data_file = open(file_name, 'wb')
   pickle.dump(data,data_file)
   file.close()                    ## should be data_file.close()

[AJG]

By "crashes" I mean my program closes. If I run it in PythonWin no errors occur.

I have changed the things you suggest, but it still crashes.

[woooee]

It works fine for me. I could only get it to crash by selecting "Use Saved Data"=1 (yes) on the first pass. Of course there is no saved data the first time you run the program, so you want to use os.path.isfile() in the load_saved function to check for a file before you try to open it.

[AJG]

Hmm... It works now that I try it again... Maybe I didn't save the changes or something =S

Thank you, how does os.path.isfile() work? I could not find it in the Python Reference Manual.

[woooee]

http://docs.python.org/lib/module-os.path.html
You can also use os.path.exists() but that will return True if you just supply just a directory name and you want to test directory + file name.

On to the bounds checking. When someone chooses what type of ship they want to build, a bomber etc., what happens if they enter a number that is not on the list? Also, you could combine ships, ships_cost and ships_shipyard into one container, probably a dictionary of lists, but that is not all that big of a deal when starting out.

[woooee]

Here is an article about users input that covers all of the bases http://linuxgazette.net/issue83/evans.html

[AJG]

Thank you, I'll follow all the advise in an attempt to make my programs better :)