Skip to content(if available)orjump to list(if available)

Flags Are a Code Smell (2014)

Flags Are a Code Smell (2014)

107 comments

·May 21, 2022

mbrodersen

I am sick and tired of “X is a code smell” articles. Everything in software development depends on context. What makes perfect sense in context A makes zero sense in context B. If (for example) all you know is front-end JavaScript development then don’t make claims about anything outside of that context. Have you ever only worked in one industry? Then perhaps realise that your experience might be limited to that context. While being completely and utterly wrong in a different context. I have worked in many different contexts in my 25+ years career. AAA game development, high-performance compilers, low-powered embedded systems, typescript/react front-end development, enterprise software applications, mobile apps etc. And the #1 thing I have learned is that nothing is true or false in all contexts. Everything depends on your particular situation and context. So be aware of your limited context and perhaps think about what would no longer be true/false when you change the context.

synu

I recall at one point a code smell meant something more like “this may be off, but not necessarily, so you should investigate and be sure in your context.” It was better than “considered harmful”, which was more absolute. That nuance may once again be lost.

patrick451

Ehh. All code smells. Especially if it was written by somebody else. Or yourself more than six months ago. Get some nose plugs and move on with life.

eurasiantiger

If code doesn’t smell like anything, it probably doesn’t do any heavy lifting.

arinlen

> I am sick and tired of “X is a code smell” articles. Everything in software development depends on context.

Your comment sounds like you didn't even read the submission. Here's, quite literally, the second paragraph:

> "In my opinion these are code smells. Smells don’t necessarily indicate actual issues, just patterns that are likely to be used incorrectly and thus warrants extra care."

You see, the author points out quite unambiguously that context is key.

ultrarunner

Then to the extent that “code smell” has become heard as “code stink” it is not useful as a descriptor. Then again, I suppose “maybe be careful with that pattern” isn’t quite the click-inducing thought leadership demanded in our present day.

arinlen

> Then to the extent that “code smell” has become heard as “code stink” it is not useful as a descriptor.

It's a very appropriate descriptor. Think about it for a second. A smell all by itself is not a sign of a problem.

> Then again, I suppose “maybe be careful with that pattern” isn’t quite the click-inducing thought leadership demanded in our present day.

Your personal assertion does not make any sense. "Code smell" is a widely-established, age-old concept. The concept of a "code smell" even precedes the concept of a "click".

onion2k

If (for example) all you know is front-end JavaScript development then don’t make claims about anything outside of that context.

I researched this throughly by reading HN and Twitter, and I can report that frontend JavaScript is the only form of software development now.

mbrodersen

Hahahaha good one :)

bb88

I've just come to hate the term "code smell". There are times when you need to take a deeper look at someone else's code, but the implementation may be completely fine given the context.

Gordonjcp

If you look at the compiler output from anything, even the "safe" languages like Rust, and I mean really get right down to assembler instructions, you know what you'll see?

Global variables and computed gotos, all over the shop.

Does it work, well, safely, and quickly? Of course.

Would you write code like that? Well, that kind of depends, eh? Is it the correct solution to the problem you have *absolutely right now* and will it remain the correct solution in the face of changing requirements?

rightbyte

The code smell concept is quite smug.

The author argues that instead of flags you could use some convoluted array of pointers to the objects for each flag to separate concerns.

Code working around dogmas is the worst "code smell" of them all.

Gordonjcp

> instead of flags you could use some convoluted array of pointers to the objects for each flag to separate concerns

Which is absolutely lovely if you have limitless memory and processing cycles. Make a sparse hash table of values to make sure you're only storing valid flags? Genius! How much memory is that going to take up?

I don't even know how much memory this modern desktop PC I have here has, hundreds or possibly even thousands of kilobytes of RAM. One of the microcontrollers I write code for has 128 bytes of RAM (this paragraph wouldn't fit) and a clock speed in the hundreds of kHz, so, if you're implementing hash tables Good Luck With That.

It reminds me of the BCS guys who reckoned that every bit of code should be written in Java, no matter what.

vinodkd

The second para of the article addresses this, as does the summary at the bottom. Maybe read the article and respond if your concerns about this particular article are still valid? Maybe it applies to the general "X is a code smell" article.

> Smells don’t necessarily indicate actual issues, just patterns that are likely to be used incorrectly and thus warrants extra care.

_gabe_

Thank you, somebody had to say it :)

cryptonector

Code smells are easier to spot in changes. When function/method arguments are added repeatedly and you realize it's time for a refactor. Or better, it's time to use monads (oh well).

dvt

Flags are not code smells and this article doesn't convince me otherwise. It's basically saying that they can be misused. Okay. Everything (from inheritance, to polymorphism, to functional programming) can be misused, so that's not really an argument against flags themselves. Flags & bitmasks (pun intended) are probably some of the most long-lived patterns in software engineering, so the argument would have to be particularly compelling, which it isn't.

Mr_P

It's not so much the existence of the flag, itself, but rather using an if-statement at the deepest-level of the call stack to conditionally modify behavior.

This talk gives a great overview of why boolean flags (rather, if-statements) can be a code smell: https://www.youtube.com/watch?v=4F72VULWFvc

OP's blogpost advocates for data-oriented design (e.g. Entity Component Systems) as a mechanism for avoiding this, whereas the talk I've linked advocates for OOP. Both mechanisms are equally valid (imho) and are inline with widely-adopted industry practices for software architecture.

gentleman11

I second that video: I’ve watched it several times over the years. I still use flags and conditionals, but less of them these days, and it’s made my life a lot easier

astrobe_

Flags are misused if their meaning is entangled, e.g. flag A is meaningless when flag B is set. In this case, you don't need flags but states, as TFA points out (with far too many words, IMO); in C lingo, when this happens you should switch from booleans to enumerations - because programmers worth their salt won't have entangled flags in something designed from scratch, it is more something that can happen when you add features.

adhesive_wombat

They also crop up all the time in embedded code, because flags are how lots of hardware works.

But, yes, it's usually a different smell in that context because you just know someone is hot-gluing the exact bitmask for a thing into some hardware-agnostic controller code without writing a proper interface that translates from an internal representation (say, an enum) to actual physical bits.

enraged_camel

You should read the article more carefully, specifically the second paragraph. Of course it is true that anything can be misused. What makes something a smell, according to the author, is that it is likely to be used incorrectly, perhaps because the distinction between correct and incorrect usage is not immediately obvious. I think the argument has merit and the example supports it sufficiently well.

dvt

The article describes an implementation detail: it's still a flag and can still be misused, but the article cleverly avoids discussing how. Component/entity-based engines (which is basically what the proposed solution is) have been around for like 20+ years. Here's me asking about it over a decade ago[1].

[1] https://stackoverflow.com/questions/4941953/the-composite-pa...

uncomputation

You don’t think the ability for a set of mutually exclusive flags to introduce illegal states (a disabled, yet visible object) into a program is a code smell? I think the array approach is far more elegant, if implemented properly (concurrently) and efficiently.

More generally, I usually take boolean flags as an indication something may be able to be factored out. Rather than a sort function with a flag alternating between LEQ or GEQ (ie sort(boolean ascending)) , I would prefer separate sort_ascending or sort_descending for example.

dvt

> disabled, yet visible object

These are bad flags and should not be mutually exclusive. "Disabled" usually applies to the physics system, and "visible" applies to the rendering system. You can certainly have something that renders but does not interact with the physics system (e.g. particle effects).

It's like throwing out all operator overloading because somebody decided to make "+" subtract instead of add.

ummonk

No that’s not really a code smell for me. It’s really normal for data types not to prohibit illegal states, and that isn’t an issue specific to having multiple Boolean flags. It’s something that a more powerful type system can help solve though (specifically, discriminated union types do this).

LudwigNagasena

I don’t see (no pun intended) why disabled yet visible should be considered an illegal state. Disabled is a higher-order concept. You can’t know whether an object should be visible or not after you’ve enabled it without storing that information somehow.

derefr

A more specific (and so hopefully less arbitrary) claim for you all:

In CRUD apps, database tables with lots of boolean columns are a data-architecture smell.

In my experience, the model encoded by such a record is almost always a — perhaps implicit — finite state machine of some kind. Usually, in CRUD stuff, this is a "lifecycle state" machine. (For example, the BlogComment lifecycle: draft → awaiting moderation → visible → deleted.)

But, because the people who create and evolve data schemas aren't usually familiar with FSMs, there's often no explicit "lifecycle state" enum-typed column in the DB record.

Instead, you get this pile of boolean columns, where each column represents the answer to a predicate that would take the model's lifecycle state as its input. Think e.g. `can_login?(User)` — which really just boils down to `can_login?(UserLifecycleState)`.

If you add the lifecycle-state column, then all the predicate-answer columns can go away. As long as you formally define+document the meaning of each lifecycle state, you can then statelessly recompute all those predicate-answers any time you like, on any layer of the stack you like. And now those predicate-answers will never be able to end up in an incoherent state, because they're never persisted; only the lifecycle-state itself is.

Of course, if you actually go all the way and make your model an FSM on the business layer as well, then you'll also get as a free benefit, the ability to validate lifecycle-state transitions — e.g. "Articles should only be able to enter the Published state from the Reviewed state."

gentleman11

I use data oriented design a bit, and also fsms constantly. I feel like fsms contract dods intentions a little: dod likes to imagine that every function just has a bunch of data it iterates over and that the logic doesn’t have to worry about application state that way. However, that is never true, and you end up adding application state variables all over your data tables so your fsms or other techniques can figure what step in a multi step process they are at for that element. At that point, it feels more and more like oop but without any encapsulation and the paradigm is starting to give me doubts. Honestly curious about this

cryptonector

It starts with one flag. A couple of years later, developments you didn't forecast have led you to adding two more flags, and now you suddenly see you need seven more and wish you'd used an enum.

This happens. It's not code smell. It's failure to forecast. An enum for one flag can be silly. Five flags instead of an enum is also silly. The latter doesn't just happen because the programmer didn't know about enums -- it happens organically over time.

derefr

What's not smelly about it? It doesn't smell of inexperience, sure; but it certainly smells like it needs a refactoring schema-migration to introduce that explicit FSM-state column.

eurasiantiger

It actually makes code a bit cleaner if data has boolean descriptors instead of enums for describing group or partial identity. Easier to check for thing.isSomething than thing.group === someArbitraryValue. Just don’t allow them to enter illegal states or depend on other state. :)

tempodox

Then write a predicate function that wraps this comparison. Extra points if you use a language that can inline the predicate, so you don't have to make a subroutine call for that.

derefr

But why not `thing.isSomething()` that's just a pure function defined as `return thing.state in STATES_THAT_ARE_SOMETHING`?

eurasiantiger

How do you store that in the database? That’s an ORM-level solution, which just hides/black-boxes the ugly code. Also, I already specified ”no illegal state”, which could just as well be ”no state”.

onion2k

A flag is just a memoized answer to the predicate though.

derefr

If you also explicitly store the FSM state itself, then sure, that's true.

But in my experience, in almost all of these cases, the flags are stored in lieu of an FSM state; with the current FSM state implicit as a constraint-solution over the current set of predicate-answers. Which allows for incoherent states, because no FSM is actually being modelled in the business logic — even though the underlying business process the code is supposed to be modelling is inherently one involving state transitions.

tempodox

The risk is that if you have mutually exclusive predicates, a memoized/persisted flag set might be contradictory. You'd need extra code to check that this is not the case. It's just safer to make invalid states unrepresentable.

smegsicle

yeah nice, way more coherent than that article seemed

jamesgreenleaf

I'm becoming convinced that using the phrase "code smell" might as well be a "smell" itself. Maybe it's just me, but I've only heard the phrase used as a rhetorical mask for a developer to express some individual preference as though it were an objective fact.

robonerd

When somebody tells you about code smell, ask them to describe the smell. If they can describe it, then you can weigh the merit of their arguments. If 'describe the smell' stumps them, brush their remark aside until they can come back with something more substantial.

Brian_K_White

Can you describe describing the smell of a code smell?

Are you saying that sometimes people say "x is a code smell" without saying what the issue is?

robonerd

> Are you saying that sometimes people say "x is a code smell" without saying what the issue is?

Oh yeah... CRs with lines marked simply "code smell", not even an "x is a".

jamesgreenleaf

Good idea. But now I'm imagining devs as sommeliers, picking out floral notes and hints of oak.

pylua

A lot of IDE plugins will tag code smell as things that may potentially be a bug in your code. I like that more objective use of the term.

mostlylurks

Those preferences come from experience. If experience tells you that code written in a certain manner consistently causes certain categories of issues later down the road, it's perfectly justified to be wary of such code.

HPsquared

More taste than smell, then.

jamesgreenleaf

Now you're making sense.

sn41

I see you haven't lost your touch.

karaterobot

One of the things I disliked the most about software development was when somebody wrote an article like this, and then somebody on my team would be like "see? you can't use flags, it's a code smell. There's an article about it, and it got on Hackernews and everything. We don't want our code to smell. Flags considered harmful. Will not approve any PRs with flags!"

Article Driven Development. People are allowed to have their opinions, but sheesh. Just say "here is a problem you can run into with flags, better be careful!"

trzy

There was someone on my last team who loved using this term in pointless nitpicking critiques. We were a prototyping team. One minute he’d wax philosophical about how prototyping should be done simple and fast, using throwaway code and cobbling together any preexisting bits we can find to move fast, and the next he’d claim that the name “NetworkManager” for the net code client component was “code smell”. Very eye roll inducing.

coinbasetwwa

The worlds full of hypocritical morons. There was a guy on my team who would promote random blog post driven ideas, but never implement the codebase. So the codebase he led was a dumpster fire, but he had enough clout that our director turned a blind eye. It would have been laughable, until our applications merged and we all had to play in the same trash pile. Everyone worth their salt left the company, so mission accomplished I guess.

onion2k

At least that's a sign that people on the team want to improve the code, even if they're outsourcing their thinking to article authors. I'd rather work with people who do that than people who don't want to improve their code at all. Using that as the starting point for a discussion is a useful process.

aastronaut

Agree here. Programming is a social discipline that should encourage discussions, rather than the enforcement of dogmas. I once worked in a team where the goal for a PR was to have as little lines changed as possible - never again.

kevinmgranger

People realized "considered harmful" was too hyperbolic, so they moved to "smell". But then it gets treated the same way, because who wants to be smelly?

colejohnson66

“‘Code smells’ Considered Harmful”

djbusby

Somewhere I saw a "considered harmful, considered harmful" article. Full meta.

synu

Unfortunately there’s article driven product management, article driven executive leadership, and probably the same thing can be found nearly anywhere someone is in over their head.

coinbasetwwa

Yep, people who evangelize blog posts have ruined many a codebase.

null

[deleted]

shrimpx

The article's most interesting point is that the vast majority of possible program (or object) states tend to be invalid. For example, `Person(isWalking=true, isAlive=false)` is nonsense, because there's an unexpressed constraint that `isWalking==true ==> isAlive==true`.

Though the author veers in another direction, I think his core point is related to this discussion:

Applying “make invalid states unrepresentable” https://news.ycombinator.com/item?id=24685772

merlincorey

> The article's most interesting point is that the vast majority of possible program (or object) states tend to be invalid. For example, `Person(isWalking=true, isAlive=false)` is nonsense, because there's an unexpressed constraint that `isWalking==true ==> isAlive==true`.

Are you implying that Undead aren't people?

Also, magical Undead states aside, I'm pretty sure it is possible for a Person to be Walking while they are not technically Alive if something else is able to control their motor system as happens in nature with various Wasps, Slime Balls, and more.

shrimpx

You make good points. Some of these "illegal" states may be legal from some other perspective, or yet-unforeseen use cases.

gentleman11

Bug or feature?

aastronaut

Also a great talk from Richard Feldman about "Making Impossible States Impossible": https://www.youtube.com/watch?v=IcgmSRJHu_8

mbrodersen

Nope. Not at all. For example, a friend of mine is working on an enterprise system used by a number of very large international companies. Every company demands slight variations to everything. From UI look/flow to how low level business logic is done. Flags are perfect for this. The key of course is to make sure that the flags are used correctly. By (for example) having a validation step at startup making sure the flags are configured correctly. Or (even better) using types to make it impossible to configure it incorrectly.

tgv

So to me that sounds less than perfect. Enterprise systems tend to be large, so it can be hard to keep track of the flags, and splitting flags can become a source of bugs. A simpler solution would be a (global, singleton) class that offers an interface to customisation. Of course, that might not be possible in an existing system, but it offers advantages over flags.

null

[deleted]

jef_leppard

If you find yourself passing in flags to change behavior, it sounds like you should just have two functions. For other cases, an enum might serve your needs better as it gives you more options as your codebase grows. That said, I wouldn't call boolean flags a _smell_. Seems like throwing out the baby with the bath water if you ask me.

amelius

Yeah, it's either flags or duplicating code. Both are smelly, so pick your poison.

jackblemming

No? Both separate functions can call a 3rd shared function if they want.

You can also restructure the code with dependency injection. There's plenty of options that don't require duplication

amelius

Alas, in reality the code controlled by the flag is often mixed with other code and separating the two is often difficult to do right, e.g. requiring to pass a lot of data hence and forth.

gentleman11

That indirection can end up a bit fractal. Sometimes the duplication is less bad than the readability reduction. You have to trust your intuition at those times

jef_leppard

Yep. You said it better than I could. There are a lot better ways to do control flow than passing in a flag. Inversion of control being another great example.

tbrownaw

Duplication is for when future changes are independent. "This client wants their reports customized" maybe.

Flags are for when future changes affect both options. "On top of the usual functionality, we can also optionally handle this other related thing" or so. Or --dry-run handling.

jef_leppard

Eh maybe. Theres no hard and fast rules in this business for sure but when I'm dealing with pure business logic, if I am passing flags in that make the function behave in more than one way, I'll try my damndest to refactor until there is one and only one thing that that function does. If I _had_ to pass in a flag, then I'd make a top level function that mostly did flow control and called into functions that had straight line logic paths. Something like this (using enums instead of booleans):

    function doTheThing({typeOfThing, ...rest}) {
        if (typeOfThing === Thing.FOO) {
           doThingWithFoo(rest)
        }
    
        if (typeOfThing === Thing.BAR) {
           doThingWithBar(rest)
        }
    }

aranchelk

> Ok, so far we have three flags which means we have 23 = 8 possible combinations of flags, but do we really have eight separate legal states? No!

It’s a good argument for a custom sum type in place of the booleans.

gentleman11

Could you elaborate?

aranchelk

Sum types let you define a type with a set of fixed possible values (a.k.a. inhabitants). You can define Boolean as a sum type:

data Boolean = True | False

The pipe character would be read as “or”. Boolean has two inhabitants. True being one, False being another one. 1 + 1

Alternatively, when you have a data structure like the example suggested by the author, it’s a product type with 3 fields, with Booleans inhabiting each (isDynamic, isVisible, and isEnabled), 2 * 2 * 2, 8 inhabitants.

The argued criticism of this encoding is that when isEnabled is false, no valid values exist for isDynamic or isVisible. They have no meaning for a disabled object.

My earlier definition for Boolean looks like an enum in some languages, but sum types can be more powerful if you’re allowed to define inhabitants with different shapes (curly braces denoting a record with named fields, “::” read as “has type”):

data GameObjectStatus = Disabled | EnabledWith {isVisible :: Boolean, isDynamic :: Boolean}

Now instead of having 8 inhabitants, you have 5 (a sum of 1 and 4) and they each have a valid conceptual meaning.

Languages with algebraic data types (sums and products) like Haskell (and IMO even better PureScript) let you easily define types that have only valid states.

sn41

Flags are a universally understood programming idiom. It is not the best way, but it is well-understood. And that counts for a lot in programming practice.

lloydatkinson

Creating many boolean states leads to an explosion of complexity and states. You can use a finite state machine to replace these states as I wrote here: https://www.lloydatkinson.net/posts/2022/modelling-workflows...

awesomedad

I notice that these code smells as discussed are often really a function of the domain the developer is in.

Lots of game examples, saw some sql too.

What for me is the main reason not to like flags/booleans, is the current hype of MVP.

Yeah, sure, a decision cam be yes or no for the current process. But what about, yes, but.... or no, unless.... for later iterations?

Also, focussing on current user requirements leads to many booleans. What about, when the process is optimal, is it still a yes/no question?

I also notice questions on different levels. In case of visible and static, for instance. One has to do (i guess) with rendering behaviour, while the other has to do with (let me call it) game engine behaviour.

Is it then maybe also a case of (re-)defining the data models, as the element you're trying to specify gains depth?

Even, possibly, that your definitions have become convoluted? In the game example, a difference between the element as an actor in a game scene as opposed to its representation for a render engine?