1
\$\begingroup\$

Right so I was doing a quick project, and sought a quick way to finish one section of my Python code. I handed it in, but I'm well aware that it's bad code, but I was wondering if there was a good way to condense this code into fewer lines.

def nGen_army(level, strength, weakness):
    if weakness == 1:
        if level == 0:
            army = round(random.randint(1000,5000),4)
            return army
        if level == 1:
            army = round(random.randint(10000,25000),4)
            return army
        if level == 2:
            army = round(random.randint(50000,75000),4)
            return army
        if level == 3:
            army = round(random.randint(100000,200000),4)
            return army
        if level == 4:
            army = round(random.randint(200000,500000),4)
            return army
    if strength == 1:
        if level == 0:
            army = round(random.randint(10000,25000),4)
            return army
        if level == 1:
            army = round(random.randint(50000,100000),4)
            return army
        if level == 2:
            army = round(random.randint(100000,300000),4)
            return army
        if level == 3:
            army = round(random.randint(300000,800000),4)
            return army
        if level == 4:
            army = round(random.randint(800000,1100000),4)
            return army
    else:
        if level == 0:
            army = round(random.randint(5000,10000),4)
            return army
        if level == 1:
            army = round(random.randint(25000,50000),4)
            return army
        if level == 2:
            army = round(random.randint(75000,150000),4)
            return army
        if level == 3:
            army = round(random.randint(150000,250000),4)
            return army
        if level == 4:
            army = round(random.randint(300000,900000),4)
            return army
\$\endgroup\$
5
\$\begingroup\$

Here's one way to do it:

from random import randint

def n_gen_army(level, strength, weakness, params=dict_of_dicts):
    """Returns random floats according to ranges specified by params."""
    if strength:
        key = 'strength'
    if weakness:
        key = 'weakness'
    else:
        key = 'default'

    lower_bound, upper_bound = params[key][level]

    return round(randint(lower_bound, upper_bound), 4)

weakness_dict = {
                0: (1000, 5000),
                1: (10000, 50000),
                2: (50000, 75000),
                3: (100000, 200000),
                4: (200000, 500000),
                }

strength_dict = {
                0: (10000, 25000),
                1: (50000, 100000),
                2: (100000, 300000),
                3: (100000, 300000),
                4: (800000, 1100000),
                }

default_dict = {
               0: (5000, 10000),
               1: (25000, 50000),
               2: (75000, 150000),
               3: (150000, 250000),
               4: (300000, 900000),
               }

dict_of_dicts = {'weakness': weakness_dict, 
                 'strength': strength_dict, 
                 'default': default_dict
                }

>>> n_gen_army(2, 0, 0)
115061.0

Some other notes:

  1. In your original code, if strength and weakness are both 1, then only the setting for weakness matters. My code preserves that, but is that behavior really what is intended? Or did you want elif instead of your second top-level if?

  2. Why are you using round? Do you really want floats returned instead of integers? If you want integers, don't use round. If you want integers rounded to the nearest 10 or 100, why not generate random integers in a 10-fold (or 100-fold) reduced range and then multiply be 10 (or 100)?

  3. Avoid capitalization in function names if you can. PEP8 and all that. All-lowercase (for functions) is good python style.

  4. You're only using one function from random, so from random import randint is better than loading in the whole module with import random.

  5. You seem to be using weakness and strength as boolean variables so if weakness: is better than if weakness == 1.

|improve this answer|||||
\$\endgroup\$
  • 1
    \$\begingroup\$ A list of tuples also works fine I think, but assumes that level will always be a non-negative integer. If this code wants extensions to level = 'infinite' or level='demigod' or even level=-1, then a dictionary is better. Whether that's a valid concern or not depends on the end application. \$\endgroup\$ – Curt F. Aug 12 '15 at 23:36
  • \$\begingroup\$ Yes, reversing the order was intentional so that if weakness: could "overwrite" if strength: since I am not returning early. That bit of the code is messy in both the original and in my version. I'm sure there's a better way to handle that logic. \$\endgroup\$ – Curt F. Aug 12 '15 at 23:40
  • 1
    \$\begingroup\$ Very late reply, but this did help a lot. I was unfamiliar with dictionaries (not sure why, they seem to be incredibly useful). I wasn't actually using weakness and strength as boolean - the number corresponds to a different section of the fictional armed forces (army, navy, air force). With some tweaking I managed to make it fit by changing the original function from which all the other parameters were generated. Again, thank you! \$\endgroup\$ – Habeeb Sep 13 '15 at 20:35
3
\$\begingroup\$

Here is declaration of a three dimensional list of list of list (of ... :) ), where the first dimension is based upon your variables, the second dimension is the level (I only added 3, not 5 as you have), and the last dimension is for the two different randint-parameters.

Have also added a print statement to list which combination is used for debug purposes, and some actual calls to the method.

from random import randint

# 1st: weakness=0, strength=1, or neither=2
# 2nd: =level
# 3rd: limits of randint - upper=0, lower=1 
riPar = [
    [ [1000, 5000], [10000, 25000], [50000, 75000] ],          # Weakness 
    [ [10000, 25000], [50000, 100000], [100000, 300000] ],     # Strength 
    [ [13, 14], [15,16], [17, 18] ]                            # none?
  ]      


def nGen_army(level, strength, weakness):

    if weakness == 1:
        idx = 0
    elif strength == 1:
        idx = 1
    else:
        idx = 2

    print ("riPar[{0}][{1}]: {2}".format(idx, level, riPar[idx][level]))

    return round( randint( riPar[idx][level][0], riPar[idx][level][1] ), 4)


print nGen_army(1, 0, 1)
print nGen_army(2, 0, 0)

Left the complete initialisation of the riPar list to you. Enjoy!

Output from a run:

riPar[0][1]: [10000, 25000]
11211.0
riPar[2][2]: [17, 18]
18.0

Code review

The key point for this simplification is to recognise the pattern you were using (i.e. always calling the randint()), remove unneccessary variable (army), and introduce a multi-dimensional list (riPar) to keep the actual difference in the otherwise equal code segments.

|improve this answer|||||
\$\endgroup\$

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.