Code Smell of the Day: Type Keys (2021)
jesseduffield.com
jesseduffield.com
Make it a callback, and now the code becomes "Here we do something specific to each subclass of users, but this code doesn't tell you what they are, there can be any number of classes and the callback can literally do anything, and if you want to figure out what's happening at this particular step, scour the entire codebase and find all the callbacks that are passed as arguments."
The code has become extensible, and thus harder to reason about. This is a good trade-off if you know you will have more and more user roles, added by different devs (or even different teams), and you want the user creation code to be decoupled from roles.
On the other hand, if most of your business logic is "Do this for customers" and "Do that for admins," then it's a useless extensibility. You will need a major change of your business requirement before a callback would be useful.
YAGNI.
No I shouldn't. I use this frequently in c++ to deduplicate common functionality that only differs by a single line. The "flag" is a zero-size struct and checking the flag is done at compile time.
struct do_bar_tag {}
template<typename do_bar_t>
int foo(int x, do_bar_t) {
if constexpr (is_same<do_bar_t, do_bar_tag>::value) {
bar(x);
}
...
}
Of course, this isn't always the right thing to do, and you can definitely overdo it. But in moderation, it's fine. "Code smell" is too often a needlessly strong opinion.
foo(int x, do_bar_t)
foo(int x, int y, do_bar_t)
foo(int x, int y, int z, int w, do_bar_t)
In languages with good support for functional features, you can curry and compose functions to your heart's content, allowing you to reuse similar logic and take better advantage of type systems like TypeScript's
But well, usually dynamicizing statically-known types yields premature abstraction
The valid complaint about the implementation I show would be that the do_bar logic is at the very top with no preamble, and it may be simpler for the caller to simply call bar(x) in that case. But adding necessary-looking complexity would detract from the clarity of the implementation (and damn c++'s verbosity, it already looks like hot garbage IMHO)
You might not "use ints as flags" but a lot of maintenance programmers who know nothing about your idea, have never seen the code before, don't know how the application works or even what it does, will do just that.
Moreover, if these unrestricted-novice-rogue-developer-maintainer-rhinoceri are going to do whatever the hell they feel like, completely disregarding the patterns and practices that I have established in my code, does it matter what I have done?
I use compile-time flags to deduplicate logic because refactoring is a nightmare if tens to hundreds of lines of code are turned into boilerplate. That is to say, I ensure conceptual integrity by maintaining a single source of truth. The nice thing about relatively dry code is that if you need to decouple it, you can easily just duplicate the code in question and run with that. If you have a dozen slightly-different copies of the same boilerplate, it's very rare in my experience for the variations to be well documented, so it ends up taking a lot of work to figure out why the variations exist -- and the real pitfall that I've seen time and time again is that a bug is found in one variant, fixed there, and not propagated to the other variants.
I love that you've called this out. So many people try and justify their coding preferences by referring to this mythical 'we' or the 'coding community' who have apparently all agreed to to some certain standard and everyone should just fall in line and get behind it without asking any questions. It's such a load of bullshit.
Fair point, but in my ~20 years of experience, I've learned what does and doesn't make for more work down the road. To wit: if something is always bad, I don't keep doing it. But I didn't just say I do it frequently, I said that I do it frequently for big-ish functions when it's only toggling a line or so. That context is important: I am not arguing that this is always a good thing. I'm saying that there's a time & place for it, and with a language like C++, it's a zero-cost abstraction.
> ...I don’t agree with the example as provided.
Yeah, neither do I. As I pointed out elsewhere, its a contrived example which can be more simply factored out into two function calls. So unless it's being used tens to hundreds of times, nothing is gained there.
Good, I'm glad. For some reason the term "Code smell" has itself become a "smell" to me.
> attributes: UserAttributes,
> onCreate: (user: User) => void
> ): User => {
> user = User.create(attributes);
> onCreate(user);
This stood out to me, in an otherwise tame article, as probably the most terrible overthought interface possible. A literal callback function passed as a function reference to something that does three lines of code? Good luck with that, I much prefer even a slightly procedual implementation.
The solution in the article could turn out even worse if the system evolves to require so many kinds of users that you end up with 8 different functions depending on what kind of user you want. You can't hide complexity of this type; you can only move it around as it emerges and try to keep things understandable.
In every design situation you're faced with countless approaches that you can take, with a lot of uncertainty as to how the system will evolve over time. I've found that the "what if" time you spend up front follows a parabolic efficiency curve: Early on in the design, you throw away some ideas that would make the first implementation easy, but would quickly become unwieldly. As the design progresses, you start thinking of more and more "what ifs", and soon you find your up-front architecture getting heavier and heavier, such that if you don't stop with the "what ifs", your first implementation will become a cathedral when all you'll need for the next year is a priest and a bench.
Clear out some easy win design issues up front, but overall don't sweat it and don't spend much time thinking "what if" because you're going to predict the future 50% wrong anyway. Refactoring isn't that hard or time intensive except in old code bases with poor hygene, and your new project is neither (unless you have poor discipline).
doAOrB(args, aOrB: bool)
@param flag: if set and if aOrB is true, warnings log to stdout and die, otherwise warnings are saved in the foobar field of args• All booleans really want to be enums
• All enums really want to be rich data objects
• All rich data objects really want to be discriminated unions
That is, whenever you write some code which accepts a Boolean parameter that drives its behavior, at some point you will regret making it a bool and inevitably need to refactor it to be an enumeration type with more than two values. But then eventually you will realize that you have one behavior in your code that applies to more than one of those enum values (all the ‘type a’ cases but not the ‘type b’ cases), and you will want those enum values to themselves have a Boolean property telling you whether they are of type a or type b (and note that that Boolean will also be subject to this same golden rule in time)
Eventually you’ll find that those different enum values (the type a and type b ones) need different sets of dependent data (type a values all have a ‘target’ as well, but type b ones don’t, say) - so you end up needing a discriminated union to capture the various data objects involved.
All of which leads us to: every if statement and switch statement (over a parameter or input value) is a disguised match on a discriminated union type. Over time, this fate is inevitable.
And if your language doesn’t have discriminated unions, learn a pattern to fake them (usually it’s the abstract factory pattern).
A practical example:
Your program starts out accepting a config file that looks like this:
enableLogging: true
logLevel: WARN
logging: {
level: WARN
file: system.log
}
logging:
fileLogger: {
level: WARN
file: system.log
}
logging:
- fileLogger: {
level: WARN
file: system.log
}
- consoleLogger: {
level: DEBUG
}You should probably generally use the least advanced of these while maintaining a pathway for future refactors into more complex versions of this
Then somewhere down the line someone realizes that actually in some cases of “forceFromDisk == true”, we still want to load the data from the cache, so the function becomes “loadData(forceFromDisk: bool, forceCache: bool)” where “forceCache == true” overrides “forceFromDisk”.
Then somewhere down the line someone realizes that actually in some cases of “forceCache == true”, we still want to load from the disk… Repeat ad infinitum.
https://news.ycombinator.com/item?id=28325563 (74 comments)
Credit: Thanks to @rosebay for pointing this out at https://news.ycombinator.com/item?id=34218449 (now dead)
Also see additional interesting submissions from jesseduffield.com:
A lot of code smells arise from this attitude. DRY is not mandatory. Oftentimes, repeating yourself is better and clearer. It allows you to grow both functions at their own pace, handle edge cases separately, prevent spaghetti code, write better & simpler tests.
DRY should only be applied to semantically identical code, not merely syntactically identical code. If the code is in the former category, and should be subject to DRY, and you're making changes to one copy, by definition you must need to make changes to the other copy … and it's a bug if you're not.
And those are the worse* kinds of repetition to encounter later on when you're trying to change the code: I've got two functions, ostensibly doing the same thing … except not. Which one is correct? (I.e., "What are the requirements?", and in my career, if I'm encountering this situation in the code, the requirements are never* documented.)
Though the initial sample code is wrong anyway - note the missing "break" statements inside the switch.
I see various methods for doing so are covered in the follow-up post linked at the top of tfa.
This is also avoidable if you have proper class/interface hierarchies, where the type "field/key" is the type itself.
In the case it's not possible and there really is only 1 type that is fully applicable to both admins and normal users then I would probably be fine with either approach (although I'd consider updating the return type to get type safety between admin and normal users!)
In these cases, it makes sense to hide those type keys from the API and only use them in the serialization/deserialization logic.
It's more loosely coupled and that always makes code harder to follow. So I'd say it could go either way. Depends on the specific example.
For the most part I feel whatever gets the job done in the least lines of code is usually the right solution.
But overall this entire exercise smells suspiciously like a constructor.
Stuff like this doesn’t bother me a ton unless it’s littered everywhere. If every function can run one of two paths that’s certainly annoying… if there is some high level function that builds one of two “pipelines” and I don’t think about userType again, I don’t care too much.
I don't read it that way. What the author is against is using parameters for the specific purpose of controlling the flow of a function. Instead, the author recommends breaking the function up into as many functions as there are variants of the type key.
This also applies to boolean flags (where only two variants are possible). There are, of course, exceptions as the author points out.
The small win is that the type key no longer needs to be maintained. The big win is that each function that gets split out has fewer reasons to change. The individual functions are therefore easier to maintain and understand.
Yet the namespace becomes more polluted and the interconnections between the functions becomes more complicated, particularly during debugging.
It's important to remember that "break big things into smaller things" only pushes complexity someone else rather than eliminating it altogether, while simultaneously increasing the overall complexity of the system by increasing the number of interconnections between the parts. That doesn't mean you shouldn't do it but you should be honest about the consequences.
> I don't read it that way. What the author is against is using parameters for the specific purpose of controlling the flow of a function. Instead, the author recommends breaking the function up into as many functions as there are variants of the type key.
Sure. How do you call the right one of the combined set of functions? A conditional branching on the type key at the call site? But then, by the same rule, the calling function should be split into multiple functions instead, and that recursisvely happens until you reach each point an instance of the typed union that might, ever, be passed (indirectly) to the function at issue is defined. And then you have a bunch of sets of near-identical functions where every function in the set needs updated for any required change that doesn’t depend on the tagged union. And the tagged union is no longer really serving the purpose of a tagged union, which is exactly to allow not having that kind of duplication.
Most of the time, DRY is better.
> its value is always known at compile time. We’re not receiving it as an parameter from an HTTP request or a value from the database.
Furthermore, from my point of view, creating an anonymous function and then store it in a variable is a bigger code smell.
def createUser(attributes, "admin") do
create(attributes) |> setupAdmin |> setupNotifications
end
def createUser(attributes, "customer") do
create(attributes) |> setupCustomer |> setupNotifications
end
# example
User.createUser(%{name: "laura", age: 30}, "admin")
I also liked the idea of using a callback for decoupling code (although that approach was discouraged by many users in this post). It could be done in Elixir as follows:
def createUserCallback(attributes, callback) do
create(attributes) |> callback.() |> setupNotifications
end
# example
User.createUserCallback(%{name: "laura", age: 30}, &User.setupCustomer/1)
User.createUserCallback(%{name: "laura", age: 30}, &User.setupAdmin/1)
def createUserCallback(attributes, callback) do
user = %User{} = create(attributes)
user = %User{} = callback.(user)
user = %User{} = setupNotifications(user)
user
end
Full code:
defmodule User do
defstruct name: nil, age: nil, type: nil
defp create(_attributes = %{name: name, age: age}) do
%User{name: name, age: age}
end
defp setupNotifications(user = %User{}) do
IO.puts("User created #{user.type}: #{user.name}")
user
end
def setupAdmin(user = %User{}) do
%User{ user | type: "admin" }
end
def setupCustomer(user = %User{}) do
%User{ user | type: "customer" }
end
def createUser(attributes, "admin") do
create(attributes) |> setupAdmin |> setupNotifications
end
def createUser(attributes, "customer") do
create(attributes) |> setupCustomer |> setupNotifications
end
def createUserCallback(attributes, callback) do
user = %User{} = create(attributes)
user = %User{} = callback.(user)
user = %User{} = setupNotifications(user)
user
end
endInside any compiler, you are going to have “type keys” and switch statements all over the place.
Even if you are writing a compiler in languages with builtin “tagged unions” and “pattern matching” you are still using “type keys” and switch statements.
Trying to write a compiler without a switch statement would lead to a giant unreadable mess.
Although the pendulum has swung far from inheritance towards composition in recent years, this is surely a prime case for simply using a subclass and instantiating whichever one you want.
The article itself is some sort of a programming flex I'd fire you for if you leave it in the code reviews too often. :P
To the author’s credit, I loved that he is thinking analytically and he did take the feedback positively - there are some good ideas in the post.
By all means use them in your work, but don’t treat it as ultimate truth. It is not.