37

This seems to be a commonly encountered and brought up code smell, and is a common feature in multiple coding design books which I wasn't fully able to understand.

Say I have a function

def train_model(model,useSGD,initWithZeros):
    
    if initWithZeros:
        model.initWeights(method='zeros')
    else:
        model.initWeights(method='random')

    if useSGD:
        model.train(optimizer='SGD')
    else: 
        model.train(optimizer='Adam')

    return model

Such a function seems to violate the Single Responsibility Principle and does different things based on it's argument flags, which I understand is bad practice.

To resolve this however, how would one refactor this? Do you create 4 functions now?

trainModelWithZerosInitOptimSGD(...)
trainModelWithZerosInitOptimAdam(...)
trainModelWithRandomInitOptimSGD(...)
trainModelWithRandomInitOptimAdam(...)

Isn't this worse for maintenance or readability? Is there any benefit of doing it like this, for this case?

Further, if I want to add a method to save model weights, called model.save_weights(...), I would need to add it to all the 4 functions, as compared to just one place?

Was hoping to learn what a good way would be to deal with these scenarios. Thanks!

6
  • 19
    SRP: Creating a properly initialised object is a single responsibility.
    – gnasher729
    Jun 25, 2023 at 23:11
  • 2
    if I want to add a method to save model weights another flag... Do you see where this leads you to? You can not encode with flags every single model state permutation. Moreover, some choices are not binary. The opposite of true is false, but the opposite of SGD is not Adam... The opposite of random is fixed, not all Zeros. Flags are code smells if they reflect this kind of design weakness.
    – Laiv
    Jun 26, 2023 at 6:37
  • 2
    why not train_model(model, init_weights_method, optimizer_type)?
    – user253751
    Jun 26, 2023 at 13:55
  • 12
    Every call e.g. foo(true, false, false, true, true) is a bug waiting to happen when someone tries to change an argument-value and accidentally changes the wrong one because they all look the same :) Jun 26, 2023 at 14:22
  • 3
    @JeremyFriesner I once had an api that needed four integers for a rectangle and a new version changed their order. A, b, c, d became b, a, d, c. I was ready to murder someone.
    – gnasher729
    Jun 27, 2023 at 16:17

11 Answers 11

96

The obvious problem

To resolve this however, how would one refactor this? Do you create 4 functions now?

  trainModelWithZerosInitOptimSGD(...)
  trainModelWithZerosInitOptimAdam(...)
  trainModelWithRandomInitOptimSGD(...)
  trainModelWithRandomInitOptimAdam(...)

What you've done here is the equivalent of rewriting:

int Add(int a, int b)

into:

int OnePlusOne()
int OnePlusTwo()
int TwoPlusOne()
int OnePlusThree()
// ...

Which is clearly the opposite of reusability.

What's the point of the advice?

Whenever you deal with any kind of good practice advice, my advice to you is to understand the reason, don't just apply the letter of the rule. Applying things without understanding them leads to many misunderstandings and code that is just as bad as (if not worse than) the original.

So why are multiple boolean arguments bad? Well, very simply put, because it's really difficult to understand the values used in such a method call once it is written.

myModel = train_model(myModel, true, false)

It is not at all obvious what these boolean settings do. Let's use another example. Tell me what we're doing here:

var result = RaiseToPower(2, 3);

Are we calculating 2³ or 3²? Well, you might be able to reason that the left-to-right order is preserved, so (2,3) must mean 2³. Yeah, that sounds reasonable. But what about this?

var result = CalculateRoot(3, 2);

Are we calculating √3 (also written as ²√3) or ³√2? Here, we come to an interesting issue. Either we retain the left-to-right nature of the parameters (which means we're doing ³√2 here), or we retain the "base number, then power" order that we used in our RaiseToPower signature (which means we're doing ²√3 here).

In either case, we are now breaking one of the natural assumptions that we made about RaiseToPower. It's always going to confuse someone.

The key here is one of reasonable readability. If there is a better way to communicate the meaning of the values, then it should be used. The goal here is to maximize readability without making it too hard to write at the same time.

Solving the core problem (solution 1)

For example, C# allows you to name your parameters. This means that you can use the name that is defined in the method signature in the method call, e.g.:

myModel = trainModel(myModel, useSGD: true, initWithZeros: false);

This already resolves the issue of readability without actually getting rid of the boolean parameters themselves. Is it the best solution? Maybe not, but I would consider this sufficiently readable.

A broader best practice solution (solution 2)

However, if focus on other good practice advice, in this case inversion of control, the solution to that would inherently also solve the boolean parameter issue.

Skipping the lesson on inversion of control, the solution entails passing the weight and optimizer into the method, rather than a flag value which then selects the related weights/optimizer:

def train_model(model, weights, optimizer):
    model.initWeights(weights)
    model.train(optimizer)

    # some really fancy logic

    return model

To be honest, your particular example is rendered to nothing when this advice is applied. I added this suggested solution anyway because it is great in scenarios where the method contains more logic than just the flag-checking logic (i.e. the some really fancy logic that I added to the example)

As a matter of fact, your example method's purpose is intrinsically tied to the passing in of boolean flags, to the point where its existence itself violates the boolean argument guideline.

Finding the right pattern (solution 3)

I think in this case the builder pattern is a more appropriate solution for you.

In essence, instead of doing this:

myModel = train_model(myModel, true, false)

Your consumer should be doing something like this:

myTrainedModel = (
    myModel
    .train()
    .withWeights('random')
    .withOptimizer('SGD')
    .run()
)

I have also opted to skip the whole stringly typed parameter discussion here, I'm not a fan of these string values but this is out of scope of the question you asked.

1
23

Functions with a bunch of boolean parameters are often "helper" functions, as in your example, that just end up delegating to other functions based on the values of those parameters. You get to write:

train_model(model, true, false)

instead of:

model.initWeights(method='random')
model.train(optimizer='SGD')

At first glance, it seems like the helper function leads to fewer lines of code. However, it's arguably harder to read because I now need to know what each boolean parameter actually does. Named parameters are marginally better, but even so, you're less likely to remember that initWithZeros=false translates to method='random' six months down the line. If you (and especially other people working on your codebase) end up needing to look up what each parameter does every time you call train_model, then that's not a good abstraction. I'd rather just make explicit calls to the model than save a couple lines of code by introducing boolean parameters.

8
  • 2
    If initwithzeroes = false means method = ‘random’ why wouldn’t you use initwithmethod(‘random’)? Boolean parameters are obviously daft if you don’t want Boolean parameters. This is not an argument against bools, but an argument against incorrect abstractions.
    – gnasher729
    Jun 25, 2023 at 21:30
  • @casablanca, Thanks for your reply. I agree that the function is essentially a helper function that performs some control logic and effectively calls other functions. In this instance, is splitting into multiple functions corresponding to each case advisable though? Or are you suggesting something of the form: if useSGD is True and initWithZeros is True: model.init_weights(method='zeros') model.train(optimizer='SGD') if useSGD is False and initWithZeros is True: model.init_weights(method='zeros') model.train(optimizer='Adam') Jun 25, 2023 at 21:32
  • @gnasher729, I agree, I should just pass method instead directly. Jun 25, 2023 at 21:37
  • 3
    @UtsavDutta: Just call the model methods directly where you need to, there's no need for any additional helper functions that translate booleans into method calls. You already have methods with more meaningful parameters (method and optimizer), mapping those into booleans only makes it harder to understand.
    – casablanca
    Jun 25, 2023 at 23:49
  • @casablanca, I see what you mean, but in this case if I find that from a caller's perspective, the abstraction of having a train_model() API is indeed useful, then I would need to create a way of calling it (as is common in a lot of major ML libraries which have a .fit() or .train() method). Another approach I came across was in Martin Fowler's blog : martinfowler.com/bliki/FlagArgument.html, where he creates a hidden implementation function which basically splits up the existing conditional paths by calling this hidden function with the 'correct' arguments. Jun 26, 2023 at 0:20
6

Prioritize client usage for simplicity over implementation issues.

Look to the callers/clients to see how they use these functions. If they all always pass constant values (true/false), then probably best to offer separate methods rather than the boolean parameters.

However, if certain clients pair an SGD and ZeroInit value together and use that for a duration (i.e. multiple times), then create an object that represents the state of SGD and init bound together for the client and pass that as parameter.

Or maybe create an object that represents the model + an sgd + an zeroinit value together, and have the client hold and use that object.  In this scenario the client has to hold onto only one variable, and can forget about the extra parameters during later method calls.

The goal is to simplify things for the consuming client as much as possible, and make things less error prone (as in passing the wrong boolean value in one of several calls).

4

In my view, if a method does a large, complex thing such as train an entire ML model, then tweaking various little aspects of that thing isn't violating the SRP.

People usually argue against boolean arguments because they make the method call very hard to read. The go-to language for ML (as in your example) is Python, which has named parameters, which takes the force out of that argument. In languages that don't have named parameters, the better solution is usually to declare specific TrainingDetails class with named fields which you can then pass to the training.

1
  • I see. The train function would again be doing the same thing though? I.e. using the TrainingDetails class attributes with some control logic to decide how to execute the training (similar to the code above). That only really changes how the arguments are passed in (as a Hyperparams object), but the function does stay the same? Jun 25, 2023 at 21:43
3

If you have a function call

F(true, false, true);

the reader is guessing what these parameters mean. Or checking the documentation. This is error prone and time consuming.

There are languages where parameters are named. If you read

F(visible: true, textVisible: false, changeAll: true);

You have a much much better idea what’s going on. At this point, having multiple functions to call becomes pointless.

6
  • Is the 'avoiding boolean usage' argument then more relevant for languages that don't use named parameters? I.e. are code smells not language agnostic? I would believe that even with Python's named parameters, this implementation is not ideal per se. Jun 25, 2023 at 21:45
  • 3
    First, “Code smell” is something that you should avoid saying in my presence. It’s like saying “you stink”. A personal attack. Second, yes, many things that are a problem in language A are not a problem in language B. Or what is called a “pattern” in language A might be built directly into language B.
    – gnasher729
    Jun 25, 2023 at 23:16
  • Understood, thanks for your help! Jun 25, 2023 at 23:58
  • 15
    "Code smell" is never a personal attack. We critique the code, never the author. The code is fixed in place by a single hash. The author is ever growing, developing, changing perspectives on historic code and on code that is yet to be written. Authors change. Authors solicit review remarks to influence how they change. Before the critique, there was room to improve. After the critique, after learning, after experience, there is still room to improve, which leads to an increasingly formidable professional who authors better code.
    – J_H
    Jun 26, 2023 at 15:03
  • 1
    "Code smell" isn't even a bad thing, necessarily. It means something in the code that seems strange, which looks like it might be something bad, but might well be good, or even perfect, for the situation/requirements. That's the reason we have the phrase "code smell". If you know it's bad, you just call it "bad" (or "sub-optimal" or whatever).
    – MGOwen
    Jun 28, 2023 at 7:54
3

I know I'm coming late to this party but I can't help myself.

Regardless of the number, and regardless of the language, even a single Boolean argument has issues. It can force you to write if code rather than use polymorphism. Maybe you like if's. Maybe you don't care about separating code into different files so it can hide from changes that don't impact it. But if you do care understand that a Boolean argument isn't helping.

As for multiple Booleans, yes combinational explosion can quickly make blindly breaking these into separate methods silly. But the refactoring replace conditional with polymorphism isn't about methods. It's about classes. And classes have the power to tame this problem.

def train_model(model, optimizer, init_method)
    model.initWeights(method=init_method)
    model.train(optimizer=optimizer)
    return model

No combinational explosion. Just sending dependencies off to the things that depend on them.


I need to clear up another misunderstanding. The builder pattern actually comes in two district kinds. The builder the Gang of Four introduced us to is not the same thing as the builder that Joshua Bloch gave us. The Bloch Builder is a Java thing. It's the one used to simulate named / keyword arguments which you don't need in languages that have them already (like C# or Python).

Even if your language doesn't have named arguments, and even if you've sworn off the Bloch Builder, there's still a simple way to handle the confusion caused by a line of Boolean arguments.

useSGD = true
initWithZeros = false
myModel = train_model(myModel, useSGD, initWithZeros)

But you don't see this very much in Python (or C#) because they have those lovely named arguments.

myModel = train_model(myModel, useSGD=true, initWithZeros=false)

Polymorphism is hardly the only way to handle this issue. If these choices are really configuration consider externalizing them in some way like making them Feature Flags. If you really like your ifs consider this method of making your choices:

def train_model(model){
  if featureIsEnabled("initWithZeros"): 
    model.initWeights(method='zeros')
  else:
    model.initWeights(method='random')

  if featureIsEnabled("useSGD"):
    model.train(optimizer='SGD')
  else: 
    model.train(optimizer='Adam')

This method is usually preferred when these choices need to be made with different deployments or builds but by people who'd rather not be messing around in the code.

12
  • 1
    "It forces you to write if code rather than use polymorphism." While true, for practical reasons, I would dissuade people from attempting to replace all boolean flags or if-statements with polymorphism. Beyond very simple examples, it results in an exponential explosion of class definitions and basically guarantees the long-term failure of a system design. I don't think that's what you are advocating for but this idea that all if-statements are a code-smell can lead to such issues.
    – JimmyJames
    Jun 27, 2023 at 15:43
  • @JimmyJames granted some people certainly over apply polymorphism. However, it only results in an exponential explosion of class definitions if you fail to decompose them. Otherwise it results in an explosion of class definitions that's on par with the alternative explosion of Booleans. I don't believe I claimed anything beyond the actual problem the Boolean arguments cause. But like I said, maybe you like ifs. I did provide an example alternative to polymorphism. Personally, I let when something is known drive how it is encoded. Jun 27, 2023 at 16:52
  • The point I am trying to make is that if you replace every if statement in your code with polymorphism, you will end up with a large number of classes. Decomposing them will not resolve that. Eliminating boolean flags with polymorphism is less problematic but essentially the same idea. At the end of the day, picking class A vs. class B is no easier (or harder) than passing a boolean value. But setting two booleans can be easier than managing 4 classes and managing 3 booleans is often easier than managing 8 classes. Knowing when it makes sense to do one versus the other is a skill.
    – JimmyJames
    Jun 27, 2023 at 20:59
  • @JimmyJames Doing ridiculous extreme things is often ridiculous. Not doing them to ridiculous extremes is often also ridiculous. Buddha taught this lesson as the "middle way". Shakespeare taught it as "too much of a good thing". True as all that is I'm not sure what you're trying to teach me here. What exactly do you want me to do? Jun 27, 2023 at 21:23
  • You are a leader. I expect you to lead which means giving good advice. You wrote: "It forces you to write if code rather than use polymorphism. Maybe you like if's". Should they like ifs? Should they hate ifs? Maybe if-statements are like beers: a few here and there are fine but too many are bad. I've seen many (usually inexperienced) developers make grandiose claims that all ifs are bad. I think there's a specific nutbag that advocates this quite militantly, but I'd need to check that claim before I get specific.
    – JimmyJames
    Jun 27, 2023 at 21:29
2

Your example is basically just boilerplate, or a "macro". It has no function of its own, and no particular benefit.

def train_model(model,useSGD,initWithZeros):

  if initWithZeros:
      model.initWeights(method='zeros')
  else:
      model.initWeights(method='random')

  if useSGD:
      model.train(optimizer='SGD')
  else: 
      model.train(optimizer='Adam')

  return model

As you noticed, this seems kind of superfluous; it combines things that should not be combined. What happens if you add a third optimizer? Not only will you have to touch this method (in addition to whatever else you need), but suddenly you need to change the type of these arguments, and so on and forth.

To resolve this however, how would one refactor this?

Possibility 1

Instead of

train_model(model, true, false)

You could simply write

model.initWeights(method='zeros')
model.train(optimizer='Adam')

There is no particular reason to encapsulate those two calls (unless there actually is a reason you haven't told us about). Assuming all 4 variations of the two booleans in this example are legal, of course. And even if some combination is illegal (for example, forgetting to initialize the weights before calling the train method), you could add exception handling for this case in train.

If you have a great many calls like this in your application, you indeed can write individual methods for it as you mention - which would not be a grand architectural decision, but just a way to make code shorter.

Possibility 2

An alternative would be to do it like this:

def train_model(model, method, optimizer): 
   model.initWeights(method=method)
   model.train(optimizer=optimizer)

If you wish to make it impossible to forget how to write the string, you could switch to constants instead (i.e., have compiler constants METHOD_ZEROS, METHOD_RANDOM and so on and forth).

This still has the drawback of combining things that should maybe not combined together, but would at least be a little better (i.e. less boilerplate code).

Possibility 3

The OO way would be to encapsulate the weights and training optimizer in their own classes (Python: modules), with a common abstract base class. And then use some more or less involved way to instantiate those classes and bring them together, i.e.

weights = ZeroWeights()
training = AdamTraining()
model.train(weights, training)

If those classes basically simply select an algorithm and require no state of their own, you can use the Singleton pattern to avoid creating superfluous objects, as well.

Final words

So it's up to you, there is a plethora of possibilities (and probably even more than I listed). All of them are somewhat a matter of taste, and depend on the usage of your code - whether it's more of a one-off; whether the functions are called millions of times vs. only one over the lifetime of the application, and so on and forth.

2

Why are boolean arguments bad? At the locations in the source that call methods that have boolean arguments, e.g. train_model(model, true, true), you cannot know at-a-glance what each boolean value means. You either have to remember, or re-check the API every single time. This is tedious and prone to error. Designing an API based on boolean arguments is also inflexible, because one day you may realise one of the parameters needs more than two values, or you may need to add an additional parameter, at which point every single consumer has to be updated too to fit the new signature.

You can replace boolean flag arguments with enum arguments. This makes the forgettable true/false litterings more immediately meaningful and readable:

train_model(model, true, false)
train_model(model, false, true)

etc. becomes

train_model(model, Optimizer.SGD, InitialWeights.ZEROS)
train_model(model, Optimizer.ADAM, InitialWeights.RANDOM)

for example.

I first heard of this solution because of the (in)famous Clean Code by Robert C. Martin, but from looking around online it still seems to be a generally well-liked pattern.


However, the best solution for your problem cross-language is normally to pass a configuration/options object that collates all of the configuration parameters like initWithZeros and useSGD, and gives them names and documents them. In Python, kwargs are normally still the best way to do this, it's a bit awkward in this language compared to others to make and use real options objects.

def train_model(model, *, useSGD=true, initWithZeros=true):
interface TrainingOptions {
  useSGD?: boolean;
  initWithZeros?: boolean;
}

function trainModel(model: Model, options: TrainingOptions)
public class TrainingOptions {
    public bool UseSGD { get; set; } = true;
    public bool InitWithZeros { get; set; } = true;
}

public Model(TrainingOptions options)
2

Your example is probably best handled by a builder of some sort as a couple of people have pointed out. But I’d like to address what is wrong with it.

Parameters are always either (a) data or (b) configuration.

Mixing up the parameter position either way is a problem, although that can mainly be addressed by named arguments in languages that support that.

Regardless, presumably if data, it’s the right data under the circumstance, and can be easily verified, so not really a major problem.

But configuration is another matter, it has several problems with it. It’s more likely to be used for sub-configuration, either alone or in combination with some other value, and it’s inherently exponential. Making it harder to test and verify at best, impossible to fully test at worst.

As configuration, one part of the program is telling another part of the program how to behave. Which introduces a tight coupling between the two. Which means that something which isn’t necessarily up to date or fully knowledgeable about what is the best decision is making it. And that is presuming it was applied to the “right” configuration in the first place. With a boolean, you have a 50% chance of getting the expected behavior if it’s applied to the wrong configuration, making it easy to get wrong and not notice it. The more boolean configurations, the more likely that misapplying it will pass unnoticed.

1

I think this is less of a problem in modern IDEs and the advice is somehow dated.

Most modern IDEs would show a function call like

train_model(Model,true,false)

actually as

train_model(Model,useSGD:true,initWithZeros:true)

using a different font for useSGD and initWithZeros.

So while on plain text, it is hard to read, it isn't really a problem any more.

2
  • 3
    While such IDEs may improve readability (in some cases), the compile time type checking remains weak, making such calls brittle should the method signature ever be refactored.
    – meriton
    Jun 26, 2023 at 15:53
  • Python also supports named arguments, so you don't need a fancy IDE to do this, just good code style. Jun 26, 2023 at 17:17
1

I agree with the other answers that the helper/wrapper method might not be necessary at all, calling initWeights() and train() separately is barely longer and much clearer than a single train_model(…) call. The boolean arguments are hard to understand, and naming them doesn't really improve things imo.

I would instead just forward the arguments and add default values:

def train_model(model, initMethod = 'random', optimizer = 'Adam'):
    model.initWeights(method=initMethod)
    model.train(optimizer=optimizer)
    return model

Instead of train_model(…, initWithZeros=true) you'd write train_model(…, initMethod='zeros'), instead of train_model(…, useSGD=true) you'd write train_model(…, optimizer='SGD').

Not the answer you're looking for? Browse other questions tagged or ask your own question.