0

I'm facing with problem that in every function (with serves as service for endpoint) I need to check what is value of query parameter (mode). I need to check it on many callables, E.g.

def create(self, arg, mode: Optional[Mode]):
    if mode:
        if mode == Mode.READ:
            do_something()
        elif mode == Mode.READ_RAW:
            do_something_else()
        else:
            raise Error("...")
    else:
        ...
    return something

My approach to tackle this problem is to create separate class like:

class ReadFacade:
    def __init__(self, read, read_raw):
        self._read_raw = read_raw
        self._read = read

def read(self, mode: Mode, *args, **kwargs):
    if mode == Mode.READ:
        response = self._read(*args, **kwargs)
    elif mode == Mode.READ_RAW:
        response = self._read_raw(*args, **kwargs)
    else:
        raise Error("...")
    return response

It's intention is to overshadow all this conditionals to not repeat it in every function like above create one. Then I can use it in many services as follows:

class SomeService:
    def __init__(self,...):
        ...
        self._mode_handler = ReadFacade(
            read=self._service.read,
            read_raw=self._service.read_raw
        )

def read(self, mode: Mode, *args, **kwargs):
    if mode:
        self._mode_handler.read(mode=mode, *args, **kwargs)
    else:    
        ...

Do I do it correctly, what I can improve? Can it be named by "facade" or it is another design pattern for that?

rozumir
  • 111
  • Does this answer your question? How to tackle a 'branched' arrow head anti-pattern? – gnat Apr 11 '22 at 10:15
  • @gnat not really, as I have only 2 ifs and no deep in stacking them. I'm more focused on finding general approach for checking this 2 conditionals for many services which have many methods like create. Like delegating this checking work somewhere else to write it only once and then use it in many places. – rozumir Apr 11 '22 at 10:20
  • 2
    The canonical approach to 90% of all problems with duplicate code is to refactor the commonalities into a reusable function. Whether that function needs to be placed in a class of its own, or not, depends on the context, the programming language, the parameter scope and to some degree on the taste of the designer. Your code looks ok to me, and when it works it is probably fine. But without seeing the bigger picture it is pretty impossible to say if it can be improved further. – Doc Brown Apr 11 '22 at 10:57

1 Answers1

1

I don't think your suggested refactoring changes anything: you still have lots of conditionals within your ReadFacade, you only separated the mode == ... conditionals from the if mode conditional. But does this really make the code significantly easier to understand? I would suspect that this extra indirection actually makes the code more complicated.

If you are concerned with having nested if/else, note that this can be easily fixed in your specific case.

Instead of:

def create(self, arg, mode: Optional[Mode]):
    if mode:
        if mode == Mode.READ:
            do_something()
        elif mode == Mode.READ_RAW:
            do_something_else()
        else:
            raise Error("...")
    else:
        ...
    return something

We might have:

def create(self, arg, mode: Optional[Mode]):
    if not mode:
        ...
        return something

if mode == Mode.READ:
    return do_something()

if mode == Mode.READ_RAW:
    return do_something_else()

raise Error("...")

To quickly address your design patterns question:

Can it be named by "facade" or it is another design pattern for that?

It is not an example of the Facade design pattern. The idea of a Facade as defined by the “Design Patterns” book is to provide an unified interface for a subsystem – instead of requiring clients to communicate with multiple different objects, they now only have to communicate with the facade which hides/encapsulates/abstracts over some sub-system. Your system doesn't really feature these different objects.

If you really wanted to factor out the mode-conditionals from your other code, you could have simply used a function/method:

class SomeService:

def read(self, mode: Optional[Mode], *args, **kwargs):
    if mode:
        self._read_with_mode(mode, *args, **kwargs)
    else:    
        ...

def _read_with_mode(self, mode: Mode, *args, **kwargs):
    if mode == Mode.READ:
        return self._service.read(*args, **kwargs)
    if mode == Mode.READ_RAW:
        return self._service.read_raw(*args, **kwargs)
    raise Error("...")

If you wanted a more OOP-y approach, the Mode itself might provide behaviour as a kind of Strategy Pattern. For example:

class Mode:
  @abstract
  def read(self, service): ...

class ReadMode(Mode):
  def read(self, service):
    return service.read()


class ReadRawMode(Mode):
  def read(self, service):
    return service.read_raw()


class SomeService:
  def read(self, mode: Optional[Mode]):
    if mode:
      return mode.read(self._service)
    else:
      ...

Compare also the replace conditional with polymorphism refactoring.

amon
  • 134,135
  • I think you did not really understand the OPs situation. From the title and and initial sentence, it should be clear the conditionals appear multiple times in different classes, with different do_something functions. They simply want to avoid the duplication, not some overengineered polymorphism solution. – Doc Brown Apr 11 '22 at 12:49
  • @DocBrown That can definitely be the case. The first part of my answer is partially about the point that not every repetition is worth extracting. If multiple functions have the same if cond1: ... elif cond2: ... pattern, then extracting just these conditionals might be more effort than spelling the conditional out repeatedly. After all, the contents of these conditionals cannot be deduplicated. Repetitive simple code is often better than DRY complicated code. The last part of the answer does provide a suggestion that gets rid of the conditionals entirely. – amon Apr 11 '22 at 12:55