Three functions to select a random character, each of which can be used at most twice

Question

I'm attempting to minimise the "duplicate" functions I have defined which do almost the exact same thing, but I'm unsure of the best way to do it, I thought of passing in the type I'm looking for (e.g. 1 for GreyLeader, 2 for HeadMarshal etc.) but that ended up looking too messy for my liking.

Could you guys perhaps suggest a way to combine the three functions into one to return the same result?

Some notes: - marshal is a list of a Marshal class which has Name, HeadMarshal, Marshal, GreyLeader boolean flags as properties - usedMarshals is a list of Marshals - each marshal can only be used twice, so the usedMarshals.count(marshal) = 2 is used for that

Code:

def get_marshal():
    marshal = random.choice(marshals)
    if marshal.Marshal:
        if not marshal in usedMarshals:
            return marshal
        elif marshal in usedMarshals:
            if usedMarshals.count(marshal) < 2:
                return marshal
            elif usedMarshals.count(marshal) >= 2:
                return get_marshal()
    elif not marshal.Marshal:
        return get_marshal()


def get_head_marshal():
    marshal = random.choice(marshals)
    if marshal.HeadMarshal:
        if not marshal in usedMarshals:
            return marshal
        elif marshal in usedMarshals:
            if usedMarshals.count(marshal) < 2:
                return marshal
            elif usedMarshals.count(marshal) >= 2:
                return get_head_marshal()
    elif not marshal.HeadMarshal:
        return get_head_marshal()


def get_grey_leader():
    marshal = random.choice(marshals)
    if marshal.GreyLeader:
        if not marshal in usedMarshals:
            return marshal
        elif marshal in usedMarshals:
            if usedMarshals.count(marshal) < 2:
                return marshal
            elif usedMarshals.count(marshal) >= 2:
                return get_grey_leader()
    elif not marshal.GreyLeader:
        return get_grey_leader()

Answer 1

1. Iterative instead of recursive

   Python doesn't really suit itself for recursion

   • No tail recursion
   • Generally slower
   • Limited max depth

2. Use ALL_CAPS for constant variables

   marshels is a constant variable and should be written in ALL CAPS

   Thnx @Gareth Rees for clarifying

3. Functions and variables should be snake_case

4. @Caridorc is correct in saying that something like isinstance() might work, but without the actual class it is difficult to tell

5. Think about changing the data type of usedMarshels

   list.count(x) is an \$O(n)\$ operation, see time complexity, meaning it will have to loop through the entire list to find how many there are.

   Instead you could hold a dictionary of the amount of marshals making it an \$O(1)\$ operation.

Here is some code to give you an idea hopefully,

import random

# Random Marshall class
class Marshel():
    def __init__(self, name):
        self.name = name

    def __str__(self):
        return self.name

a = Marshel("First Marshal")
b = Marshel("Second Marshal")

MARSHELS = [a, b]        
used_marshals = {a: 2, b: 1}

def get_leader(type_):
    while True:
        # marshal = random.choice((m for m in MARSHELS if isinstance(m, type_)))
        marshal = random.choice(MARSHELS)
        if marshal not in used_marshals or used_marshals[marshal] < 2:
            return marshal

if __name__ == "__main__":
    print(get_leader("Does nothing yet"))
    # Will print "Second Marshal" because that is the only viable option

Answer 2

Solution

def get_something(marshal_etc):
    marshal = random.choice(marshals)
    if marshal.marshal_etc:
        if not marshal in usedMarshals:
            return marshal
        elif marshal in usedMarshals:
            if usedMarshals.count(marshal) < 2:
                return marshal
            elif usedMarshals.count(marshal) >= 2:
                return get_something()
    elif not marshal.marshal_etc:
        return get_something()

You could just make a generic function that takes an input of all your options, you can do this by replacing any occurrence of those options with a generic label marshal_etc or whatever you like.

Then just run get_something(HeadMarshal) or get_something(Marshal) or get_something(GrayLeader) as needed