2

I have some functions that have implementation details that depend on which type of object is passed to them (specifically, it's to pick the proper method to link Django models to generate QuerySets). Which of the two following options is the more Pythonic way to implement things?

If ladders

def do_something(thing: SuperClass) -> "QuerySet[SomethingElse]":
    if isinstance(thing, SubClassA):
        return thing.property_set.all()
    if isinstance(thing, SubClassB):
        return thing.method()
    if isinstance(thing, SubClassC):
        return a_function(thing)
    if isinstance(thing, SubClassD):
        return SomethingElse.objects.filter(some_property__in=thing.another_property_set.all())
    return SomethingElse.objects.none()

Dictionary

def do_something(thing: SuperClass) -> "QuerySet[SomethingElse]":
    return {
        SubClassA: thing.property_set.all(),
        SubClassB: thing.method(),
        SubClassC: a_function(thing),
        SubClassD: SomethingElse.objects.filter(some_property__in=thing.another_property_set.all()),
    }.get(type(thing), SomethingElse.objects.none())

The dictionary option has less repeated code and fewer lines but the if ladders make PyCharm & MyPy happier (especially with type-checking).

I assume that any performance difference between the two would be negligible unless it's in an inner loop of a frequently-called routine (as in >>1 request/second).

cjm
  • 451
  • 4
  • 20
  • 2
    It seems like the problem here is *the need to do that* - if the subclasses have such different interfaces, you're breaking the LSP. – jonrsharpe Nov 23 '19 at 19:34
  • 3
    @jonrsharpe has a nice point. on top of that, if you declare the dictionary that way, you will always execute all the branches and it's gonna fail badly. Those should all be non-applied methods. My suggestion is to rethink the whole structure to avoid having to make this piece of code entirely. eventually move more logic inside the various SubClasses and hide everything behind a common method. – Chobeat Nov 23 '19 at 19:38
  • Would a better organization be to declare `do_something` as an abstract method in the superclass and then implement it in each of the subclasses? – cjm Nov 23 '19 at 20:09

1 Answers1

2

This is exactly the type of problem polymorphism aims to solve, and the "Pythonic" way to solve this problem is to use polymorphism. Following the notion to "encapsulate what varies", I'd recommend creating a base "interface" that all classes implement, then just call a method of the same name on all classes.

I put "interface" in quotation marks, because Python doesn't really have interfaces as they're commonly known in OOP. So, you'll have to make do with using subclasses, and enforcing the method signature manually (i.e. by being careful).

To demonstrate:

class SuperClass:

    # define the method signature here (mostly for documentation purposes)
    def do_something(self):
        pass

class SubClassA(SuperClass):

    # Be careful to override this method with the same signature as shown in
    # SuperClass. (In this case, there aren't any arguments.)
    def do_something(self):
        print("Override A")

class SubClassB(SuperClass):

    def do_something(self):
        print("Override B")

if __name__ == '__main__':
    import random

    a = SubClassA()
    b = SubClassB()

    chosen = random.choice([a, b])

    # We don't have to worry about which subclass was chosen, because they
    # share the same interface. That is, we _know_ there will be a
    # `do_something` method on it that takes no arguments.
    chosen.do_something()
jknotek
  • 1,778
  • 2
  • 15
  • 23
  • 1
    I'd make the base class `raise NotImplementedError()`, rather than silently `pass`: https://stackoverflow.com/q/44315961/3001761 – jonrsharpe Nov 24 '19 at 08:48