1
\$\begingroup\$

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()
\$\endgroup\$
  • 1
    \$\begingroup\$ Maybe get_leader(kind): leader = random.choice( (m for m in marshals if isinstance(m, kind) ) ) I cannot test it without the marshal classes. Where kind is marshal.Marshal or marshal.HeadMarshal or marshal.GreyLeader \$\endgroup\$ – Caridorc Aug 28 '18 at 11:07
2
\$\begingroup\$
  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
\$\endgroup\$
  • \$\begingroup\$ According to PEP8, ALL_CAPS is only for constants; global variables should be named like functions, that is, in snake_case. \$\endgroup\$ – Gareth Rees Aug 28 '18 at 12:05
  • \$\begingroup\$ Thanks! With the isinstancetype(), if the Marhsel class had a property of 'type' as well as its 'name', and wanted to determine if its type was 1, would I then have three different classes and use them like 'Marshel(type_1)' and then isinstancetype(marshal, type_1) to determine if its type one? Or can I do something easier? \$\endgroup\$ – user132955 Aug 29 '18 at 0:28
1
\$\begingroup\$

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

\$\endgroup\$

Your Answer

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