Flags Are a Code Smell (2014) | Hacker News Reader

Flags Are a Code Smell (2014)

sebastiansylvan.com

58 pointsspekcular102 comments

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.

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.

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.

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

Hiding complexity by smearing it out over abstractions and more loc:s is the name of the game.

In my experience, complexity arises from a mismatch between data structure and data usage patterns, and it can be eliminated by changing the data structure to match its usage.

Ye. But if the problem to solve is complex or convoluted enought so will the solution be. Somewhere the code has to actually do something no matter the approuch.

> 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.

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.

> 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".

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.

Hahahaha good one :)

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.

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?

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.

> 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.

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.

Thank you, somebody had to say it :)

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).

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.

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.

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

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.

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.

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.

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...

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.

> 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.

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).

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.

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."

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

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.

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.

Yes.

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. :)

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.

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

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”.

> How do you store that in the database?

1. A very small dimension table, defining the set of flags in terms of the current state: (user_lifecycle_state, is_a, is_b, is_c, ...). Join it to the fact table when you need it.

2. Computed columns in a writable view wrapping the table.

3. Procedures/functions that take the lifecycle state as input, so you can `SELECT u.id, ..., is_active(u.lifecycle_state) AS is_active FROM users`.

> which just hides/black-boxes the ugly code

I don't see what's ugly about it. The point of defining something as a FSM is that that's a formal definition on the design level, separate from any particular implementation; you can write a single stateless module that defines these predicates in terms of the lifecycle-states on any or all layers of the stack they're needed, and they'll all work the same, because they're all working off a formal definition of the meaning of each state.

Compare and contrast: Postgres has an `inet` type for IP addresses. It has a function `family(inet)`, which returns 4 or 6. Your business layer's runtime probably also has a function like that somewhere. They won't necessarily have the same outputs (i.e. your business layer's version might be a pair of predicates `is_ipv4?` and `is_ipv6?`) — but they'll both answer the same way semantically given the same inputs, because they're both working off of a shared formal definition of what an IP address is.

Thanks for the clarification. The example, however, doesn’t fit reality particularly well given how few implementations actually agree on all possible forms of v4 and v6 addresses.

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

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.

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.

yeah nice, way more coherent than that article seemed

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.

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.

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?

> 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".

Hah, wow. I'd have some fun with that. "Is it blueberry?"

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

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.

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.

More taste than smell, then.

Now you're making sense.

I see you haven't lost your touch.

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!"

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.

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.

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.

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.

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?

“‘Code smells’ Considered Harmful”

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

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.

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

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

> 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.

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

Bug or feature?

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

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.

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.

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.

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

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

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.

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

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.

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.

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)
        }
    }

> 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.

Could you elaborate?

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.

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.

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...

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?

Most of the individual boolean flags in the code I work with are retrieved on-demand from a feature-flag store. Well, aside from the dryRun flag that a lot of utility things take.

But this actually seems to be talking about having object state represented internally as a pile of flags when something more semantically meaningful is available.

I'm not sure I agree with this article. I have been looking at the source code of some open source games and they all make use of a mix of flags and some sort of polymorphism. At some level, when you get into a low enough level class, there is most certainly a ton of if, else, and flags. Not everything can be normalized elegantly into polymorphism, because there are too many combinations.

I wouldn't describe these open source games as having code smell, but rather just being very complex. Using flags seems to be a very effective way of encapsulating the complexity.

I have not looked at a popular open source game that has really used the approach described in the article. Does anyone have any references for that?

Games are a bit of an atypical case, since the overhead of runtime polymorphism (allocation, virtual function calls) motivate many to avoid runtime polymorphism due to the importance of performance in games, regardless of if that motivation is justified or not. Thus, polymorphism might still have been the most elegant approach, but its use avoided due to non-stylistic factors.

I feel like this is true, but just anecdotally. Is there any good literature on the cost of if/else vs polymorphism? Does branch prediction really work that well in practice vs polymorphism? I figure the approach in the article would potentially have bad cache locality, but I am unsure of how it would perform.

Isn't this essentially comparing switch/jmp table to a vtable?

The switch/jmp table can be optimized to be inline, vtable will likely incur a stack frame, plus usually 3 indirections.

When you have a closed set of types, then you know all cases for branching, in polymorphism you lose this power -- it's open ended.

This is another reason why languages should have exhaustive pattern matching with sum types.

Function pointers / vtables / polymorphism are slow, because they're indirect jumps (harder to predict). Function calls are slow, because there's work to do to save / restore state on the stack.

These don't matter much for big heavy functions, but tiny ones can seriously benefit from being inlined.

I have heard elsewhere that "game objects are just an index into a bunch of arrays for all the object properties" is fairly common practice, but well I don't write games myself.

I don’t like using them, but, sometimes, they are required.

That pretty much sums up all kinds of “smelly” practices.

In my experience, “God said it, I believe it, and that settles it” type pronouncements end up looking fairly silly. We do what needs doing, to achieve our ends.

At least, that’s what most experienced ship programmers do.

What I prefer (I program in Swift), are computed properties that examine the current state, and synthesize a report on demand.

Can’t always do that, though. Things like performance and threading can be factors. I just ran into something like that, tonight. Not sure how I’ll solve it. I’ll sleep on it.

flags are fine. If they reduce complexity. Is it easier to add a comment about what the flags do for the next dev than describe the other structures to avoid the flags? You won't the last dev to touch that code. Write the simplest thing possible.

There was an opportunity here to speak about finite state machines.

If the code ain't smelling, it ain't working!

Putting stock in code smells is a process smell

A bit hyperbolic calling code smell. They even mention they have their uses.

I think the main point of this post is be careful with state explosion since each flag grows the number of states exponentially and likely not all of then are legal or make sense.

The other point is grouping similar things in memory is good for performance. Less failed branch predictions from checking flags and better cache coherence.

Thats not as a click bite-y of a title though.

Although the above is sound advice.

Shit. I guess I should remove the flags from my models.

But now how do I tell if a company is a qualified vendor or not? If a user is active and can login.

I could have sworn I just read an article about putting code that's not ready behind a feature flag instead of long running git branches... guess that was a fever dream?

Pre-procs are knocking at the door... and it's raining out.

Article about nothing. Take feature A and declare it code smell and the say that in many cases it is actually not. In programming any single feature / concept is like that. Good for one case, bad for the other.

Reply on news.ycombinator.com