Anti-pattern in which code blocks are indirectly used as parameters

Question

I was recently trying to explain a particular anti-pattern to some novice programmers and found that it was hard to express without an overly-detailed example. I'm sure it has a name and that someone has spent the effort to write a succinct/clear explanation of it, but I have struggled to find it.

The anti-pattern usually starts with a function:

def make_model(input_size, output_size):
   "Builds a neural network model from the parameters and returns it."
   # ... <initial setup> ...
   # Build a 4-layer neural network
   model.add_layer(some_conv_layer(input_size))
   model.add_layer(some_conv_layer(input_size * 2))
   model.add_layer(some_relu_layer(output_size * 2))
   model.add_layer(some_sigmoid_layer(output_size))
   # ... <etc> ...
   return model   

Then, at some point while developing the software, we realize that, in fact, the number of layers and the kinds of layers are a parameter of the model, and we should handle that because sometimes we want a different set of layers. So we update hte function:

def make_model(input_size, output_size, model_arch):
   "Builds a neural network model from the parameters and returns it."
   # ... <initial setup> ...
   if model_arch == 'basic':
      # Build a 4-layer neural network
      model.add_layer(some_conv_layer(input_size))
      model.add_layer(some_conv_layer(input_size * 2))
      model.add_layer(some_relu_layer(output_size * 2))
      model.add_layer(some_sigmoid_layer(output_size))
   elif model_arch == '5layer':
      # ...
   else:
      raise Exception("invalid model architecture parameter")
   # ... <etc> ...
   return model   

However, as the code development progresses, this becomes untenable--every possible architecture of the model must reside in this one function, and parameters of the every model architecture need to be made to somehow route through this make_model function.

Depending on the programming language you're using and the paradigms to which your codebase is adhering, the solutions to this problem vary (in Python it likely involves classes), but I'm interested in the name of the anti-pattern and in documents describing it broadly for novices: how to identify it, strategies for fixing it, consequences of not fixing it.

Thanks in advance.

Answer 1

Not all bad code has a predefined type or name. Sometimes, it's easier to express what the code does wrong instead of what arbitrary name we've chosen for it.

This code violates the Open-Closed Principle, which states that objects should be open for extension, but closed for modification.

Whenever you extend your codebase (i.e. adding an extra kind of layer), you have to modify this class, rather than extend it.

Comparatively, if you were to create a system where new layer types can be created (not using magic strings like here) and they have handlers (essentially classes that represent the logic in each if block in your example) are registered to this class, then you are relying on extension instead of modification. That would follow OCP.

In such a case, this class would not need to be changed when you create a new layer type. Your outside consumer simply registers a new handler (for the new layer type), and this class receives and interacts with it without needing actual changes to its own logic, therefore no longer requiring what OCP refers to as modification.

Answer 2

This looks like a factory method of which is not necessarily an anti pattern. You're always going to need an if-condition somewhere, and the purpose of factory method is to centralise the scattered condition check into a single place. If this is the only place in your code where you need to check model_arch, this isn't an anti pattern at all and the factory method had done its job. Don't overengineer things by going to ends of the earth to eliminate this branch, you can move if-condition around and you can merge duplicated conditionals, but you rarely can remove them completely. The cure can end up being worst than the disease if you try to forcibly remove conditionals statements from a properly functioning factory method.

There are cases however when the factory method hadn't completed the job. If you have multiple places in your codebase checking for model_arch == "blah" and you ended up having to pass model_arch (or some other value that correlates with it) around to places that need to behave differently, then that factory method hadn't completed its job. You should continue refactoring until there's only place that checks for the architecture.

Also, if the factory function ended up getting unwieldy and contains dozens of these, and they keep changing all the time, or if they need to be updated by users of the system, then it may be desirable to redesign the definitions so they're read from a data file or a database rather than keeping them as code. However, this is definitely the over engineering route most of the time, don't go this route just because you simply want to move the branches around.