Code Smell of the Day: Type Keys | Hacker News Reader

Code Smell of the Day: Type Keys

jesseduffield.com

85 pointsjesseduffield74 comments

This is sleight of hand. The original code provides a uniform interface to create a user, with a parameter to distinguish the case. This functionality has disappeared in the refactoring. However, a requirement for this case-distinction must still exist somewhere else in the code, likely multiple times, otherwise we would not have a need for the original function in the first place.

One thing you should avoid for readable code is callbacks and interfaces. This may run counter to advice you may find in books, but programming practice should've taught you that use of these features makes it exponentially harder to figure out "what's going on here". They are tools to solve specific problems, not defaults to apply liberally.

> However, a requirement for this case-distinction must still exist somewhere else in the code, likely multiple times, otherwise we would not have a need for the original function in the first place.

You must work in a nice place - I often find code that does not have an ideal layout.

Of course, code that has been speculatively laid out for a use-case that never materialized is not uncommon - a good candidate for refactoring. However, code speculatively laid out for a use-case that did materialize can prevent a refactoring. One should apply the YAGNI principle - judiciously, not dogmatically.

For example, in this case there are really only two user creation types, which could be modeled with just an "isAdmin" flag. However, the likelyhood that a third type (or more) will appear down the road is high, so it is reasonable to speculatively use an enum-like type here.

I don’t think that’s the gp’s point.

Every call site has to decide whether the type argument is admin or user. After the change every call site has to decide which of the two create functions to call. The differentiation already existed for a reason not explicit at the place where create* was defined (although in this trivial,example it’s obvious).

This is what struck me. All the "refactor" did was move the decision of what function to call somewhere else instead of code presented to us.

I'm afraid I'm not smart enough to deliberately use sleight of hand in one of my posts, but I take your point. I'll update the post to address your criticisms and others from this thread.

My main motivation for writing the post was seeing type keys used for truly no good reason and leading to code that was impossible to understand, but the post could spend more time considering when type keys are appropriate and the downsides of alternatives.

Like others have noted, it is actually harder to read and grok the code when your function is able to do literally anything. Before, you got what you saw. After, the callback might as well set off WW3 for all you know. This fits poorly with the philosophy of doing one thing well, and ironically makes it much harder to create an admin user - you have to know what the correct callback is, as opposed to noticing an enum type or a bool type.

I would probably create two functions, createAdmin and createCustomer, then as the post briefly does, factor out commonalities where it makes sense. Note that factoring out ALL commonalities is often a red herring, some duplication may be preferable to strangely factored code (eg the callbacks…)

it's funny because factoring out so called commonalities sometimes results in even more code that can actually be harder to read than the initial mess

> Why Is This Solution Better?

It is not.

This kind of refactoring reminds me of a developer we used to have who would do this sort of thing to the entire codebase. Every slightly-overweight method that took care of an entire concern scattered to the seven winds of "best practices" philosophy. Rinse and repeat enough times and you find yourself taping source code printouts to the wall and tying strings between them like its a CSI episode.

As someone who spends a lot of time looking at ASTs it feels slightly naive to write off type keys completely. They are particularly useful in TS because it uses flow analysis to isolate the correct interface pattern from a union of types. Although I will generally encourage using conditional flow to direct to variant specific functions instead of mixing types in the function body.

I would heartily support the refractors given in the examples if they reached me in an MR. But chances are that a type key is introduced in 1 location, and then acted on in 6 different locations with a large temporal distance. Without the type key you are likely to need some way to infer the type, which may be error prone and hard to maintain.

On a micro scale a common example is where you have to pass over a list of statements to declare the functions, and a second time to define them. So that they can be self referential. Preferably without N scale memory allocations caused by things such as filtering the list.

My first thought was ASTs and union narrowing too! Probably because that’s been an area of focus for me lately too. Of course there’s other ways to represent a discriminated union, but type keys are particularly useful and usable for AST tools which are designed to be extensible (unified, estree come to mind).

One significant downside, which I think would better support the article’s point, is that type keys make composition (or inheritance if that’s your thing) more awkward. To represent an intersection type, you need either a nested structure (more flexible) or implicit relationships between types (please do not).

It’s a more generalized solution, that’s for sure, but it feels like it adds a lot of complexity and indirection in the process. The “flow of code execution” is all around with the callbacks, and it took me a few back and forths to actually understand how it was working.

Now, if you have a large system and many more of these “type keys”, by all means, this is an appropriate solution. But in the example of user type / creation, I beg to differ: the earlier example is much simpler to understand and reason about.

Agreed. Another situation where type keys aren’t the grinch is de/serialisation. A contrived example: it’s somewhat simple to imagine the first example as half of a request handler, taking in some string input, asserting it’s within the set of type keys, then acting on it. Later examples, not so much. IMO a great example of “don’t let the perfect be the enemy of the good”

This article also inadvertently shows what I think is a big drawback of the inversion of control pattern.

Imagine you are trying to debug an issue with user creation.

In the last example, you would have to look up everywhere `createUser` is being called, and follow the code path through several different scattered files until you find your issue.

In the original code, you can simply look up `createUser` and you have the complete code flow in front of you.

This comment speaks to me. I like to be able to follow code flow to figure out what's going on, and that means I prefer "from here to <somewhere>" rather than trying to figure out "to here from <somehwhere>". Mind you, sometimes passing in a function is the right answer, but I try to limit it to things more like higher order functions than "adding logic to an existing method".

An arguably better solution, at least in statically typed languages, is to make the type keys constant objects that implement a `setup(user)` method, which for example is possible in Java by using an enum type for the keys. E.g.:

    enum UserType {
        ADMIN { void setup(User) { … } },
        CUSTOMER { void setup(User) { … } },
        ;

        abstract void setup(User);
    }

and then have:

    User createUser(UserType type, UserAttributes attributes) {
        User user = new User(attributes);
        type.setup(user);
        user.setupNotifications();
        return user;
    }

Sometimes you can make the dependency graph look nicer, but make your code less readable. Compare the example with the "isPremium" flag, and the example with callbacks. I've seen many times when a nice readable code becomes a garbled mess after a refactoring to make it more architecturally sound. What's more important?

Ye the concept of "code smells" is a code smell. What exactly is wrong with explicit and clear handling? Too long functions for Clean Code? He just trades vertical function length for depth and depth is harder to keep in your head.

The trade-off is with future evolution of the code. If the common logic changes in the future, then with the first version it has to be updated consistently in multiple places, because it duplicates the common logic. In practice it happens quite often that such code is modified/evolved inconsistently.

The more explicit version may still be the better trade-off, but in general it’s not a clear-cut question.

Sure. Either way might be better depending on circumstances. It is when rule of thumbs are applied dogmatically the result suffers. And there are alot of dogmatic programmers in my experience.

Eg. I have noticed some collegues want a functional programming like flow of constant value objects, but they don't think about that they are essentially storing the state in the code path instead. Etc.

This code is hilarious. He took the switch statement out of one function and then put it in another. The only difference is he didn’t show the new function he put it in.

Very likely, he took the switch out of a central function that's only written once and placed it into a few of them scattered through the code.

With time, somebody will probably encapsulate his code in a flexible "createUser" function on an upper level, so the switches can all come back into a single place.

This is smearing code around to distract from what is likely short-sighted IAM data architecture.

"Admins are Users except not really"

If Admins are really Users, then create a User first and then elevate to Admin with ACL assignments, etc. There should probably be accompanying inverse procedures too.

If Admins are not really Users (i.e. there is a strong segregation between internal "users" and customer "users"), then avoid coupling their logic at all.

People have a weird understanding about what makes code understandable, readable and maintainable.

Exactly. It’s a subjective topic. When I was younger I used to think in terms of “good code” and “bad code”. Nowadays it’s all about “zero code” and “some code”: the best code is the code that’s never written. I leave style issues, good practices, SOLID, etc. to the younger ones to discuss about it, it does not matter anymore.

I see this attitude a lot that after 5-10 years devs think they no longer have to worry about writing maintainable code.

I respect that once you reach a certain level of competence you can probably coast by without thinking too much, but let's not pretend comfortable stagnation is the same thing as mastery.

Heh. I feel that all that mumbo is interesting. But for the most part it’s just dogma. I got my own dogma, but I’m not selling books about it and I’m not interested in teaching anyone. I also don’t care about anyone else’s dogma, and pretty much dismiss it if you try to use it as an argument in a code review.

I’m writing code to solve problems, I’m not here to discuss the abstract art of code patterns and structure.

This isn't about "abstract art of code", the original comment was that the guy literally didn't care about best practices or code style. Like it or not part of the job is writing code that the rest of the team can read and understand. Ignoring these things is just showing a lack of respect for your co-workers

25+ years of experience here. I see this problem with inexperienced developers much more than experienced ones. Which makes sense. Experienced developers have more experience feeling the pain of having to deal with unmaintainable code. So they care more about it.

It's not as much "subjective" as it is "context dependent".

Take two people with similar experience on the language and idioms you use, and they will very likely agree on what code is easy or hard to understand.

Yeah this is just bad. It went from being readable to the point where you cannot tell which function is called and where. The original type key is fine.

What I often wonder is why, despite all kinds of languages experimenting with syntax, there isn't more innovation with function/method signatures.

Seems to me, a lot of "DSL"/"API design" work is really about passing some compile-time data structure to a function that is too complex to be represented as a simple list of parameters.

A lot of times, this is done by abusing language features or by setting up a runtime data structure that is then immediately unpacked again inside the function.

In the most extreme cases, you design a custom DSL, have the caller pass expressions in that DSL as a string or file handle to the function and then parse the string inside the function.

All of that causes a lot of runtime complexity and overhead for what is essentially static data.

So wouldn't it make more sense if you could define a DSL in a function signature and connect it with preprocessor/macro statements inside the function? This way, the compiler could parse the DSL during build and we could get rid of all the runtime overhead.

Example: A fictional function definition could look like this: (where # indicates a keyword destinied for the preprocessor)

  public Response fetch(String url, Map headers, (#keyword method=GET) #or (#keyword method=POST, byte[] body)) {

    // ... common code
    #switch (method) {
      #case GET:
        // ... GET-specific code
      #case POST:
        // ... POST-specific code
    }
  }

A compiler would generate two separate methods from this definition. (To avoid C macro madness, you'd probably first generate an AST, then have the preprocessor modify that AST.)

A call site could look like this:

  result = fetch("example.com/", {}, GET);

  result = fetch("example.com/", {}, POST, data);

Which would call one of the two methods in a fashion analogous to method overloading.

Rust has this with its macro_rules! macros. It's not as good of a solution as it initially seems.

Interesting to know. What were the issues?

Mostly the syntax is bad, I think. Also, exposing the AST like this means implementation details about the parser are now locked in.

I can understand the syntax bit. For that feature it's really important the syntax is accessible. (I have no illusions how bad my syntax above was. I just threw it together to illustrate the point)

Disagreed on the AST though. I think a lot of problems can be solved if a programming language has a well-defined publicly available grammar and if access to its AST is easy. As an example, a lot of innovation in modern IDEs comes from the IDE's ability to actually parse the code files and build object graphs. Knowing details about the AST is necessary for that.

Knowing details about the grammar is necessary for that. Rust's macro_rules! (and, to a lesser extent, your syntax) exposes implementation details about the compiler as parts of the standard, so it's difficult to use (e.g.) a different architecture of parser.

> and, to a lesser extent, your syntax

That was a fantasy syntax I just made up. How can I expose implementation details of a compiler that doesn't exist?

Looks like the author is 're-functionalising' (as opposed to https://en.wikipedia.org/wiki/Defunctionalization ).

Defunctionalised programs pass around a simple data type, which is branched on in various places to implement different behaviours. Re-functionalised programs pass around different behaviours (as functions, or objects if you insist) which are called in various places.

Neither form is strictly better than the other; it depends on the problem we're trying to solve.

The author doesn’t mention it in the article but the pattern suggested is inversion of control (IoC) which is one of my favorites. Highly suggest looking into it if you’ve not heard of it.

Yeah this is the "D"[1] is SOLID [2]. Robert "Uncle Bob" Martin talks about this in his books Clean Code and Clean Architecture. I'd recommend them. Clean Architecture is really good as an audio book.

[1] https://en.wikipedia.org/wiki/Dependency_inversion_principle

[2] https://en.wikipedia.org/wiki/SOLID

I can't see the Dependency Inversion ("Depend on the interface not the implementation") in this?

Think of it like this:

I am in injecting a function that returns a `User` type. That is the interface. I, the function accepting the injected function, don't care how it gets that `User` (that's the implementation). I just care that I am getting a `User`.

Now, knowing that, I can inject any number of function implementations that implement that interface. As long as a `User` gets returned it does not matter how the sausage gets made so to speak.

I wish my competitors will spend more time (ideally all their time) fixing “code smells” in their code. It will make it even easier to beat them in the marketplace. I once fired a developer who didn’t understand how to prioritise his work. It improved the productivity of the team (him not wasting everybody’s time with low priority nonsense).

Describes control coupling with a different name, “type keys.”

This refactoring pretty well walks down the path of the expression problem [1]. Sometimes it's more convenient to have a fixed set of functions that accept different types, and others its more convenient to have a fixed set of types that offer different functions. One's not strictly better than the other.

[1]: https://craftinginterpreters.com/representing-code.html#the-...

Why not use something in the form of SingleDispatch on the typed key?

This is a very simple pattern with Python, instead of passing literals, create the different literals as types, and make the function accept a union of said types. Then use `functools.singledispatch`, to map each type to the correct function. This results in three separate functions, just as one would do with algebraic data types.

Or, instead of singledispatch, use dictionary mapping `type(key)->Callable`, and pass the arguments there, making the graph identical to the second.

Or, maybe the original code was actually totally fine and in fact the simplest and most obvious solution.

Literals should be avoided since they can get mistyped unless the compiler can enforce that.

The TypeScript compiler can enforce that, notice how the type annotation in the example is `'admin' | 'customer'`, not `string`.

It's also possible to give this type a name:

    type UserCreationType = 'admin' | 'customer'

I see. Then I am joining the club where the first one was perfectly fine.

Yup, typescript is pretty good when it comes to this kind of stuff. This was one of the very few things I tried when I first started with TS.

The thing about boolean flags is interesting. My gut reaction was that it was highly questionable as a general principle, because you might have several such flags and you don't want to make a separate function for every combination.

But in a case like this, where a flag or enum is dispatching between two distinct code paths, maybe it's worth considering separation into distinct functions.

This is a Hot Take (tm) that I havent fully considered so grains of salt etc, but boolean flags are in essence bad product type. Given a function

  create(entity, bool_a)

we have a function that creates an entity and take a bool flag, and it has 2 possible variants (entity + true, entity + false). If we add another bool flag

  create(entity, bool_a, bool_b)

then it increases to 4 states. Add another and you're dealing with 8 states, and so on and so forth. We likely don't care about all 8 states - rather 4 or 5, but complexity increases.

In this case, with the type key, we're essentially dealing with a sum type that has two variants

  (admin, attributes) | (customer, attributes)

which is fairly clear. if we add another variant to it, we only increase by one

  (admin, attributes) | (customer, attributes) | (distributor, attributes)

I agree that in this particular example of creating an instance of an entity, it's bad. It doesn't even make sense, like something you would set up as a strawman in a post about being too DRY.

What would be the point? To try and ensure that certain setup measures are taken in all cases?

Do people actually write code like this? Maybe this is a programming language culture difference, but I never see this pattern in my Python work, professional or otherwise.

I think my issue is that this post takes a very narrow view of what Boolean flags are used for. I would hope that they are not advocating against things like `fetch(url, verifySsl = false)` !

In Python libraries (pyyaml, for example), occasionally you’ll come across a pair of methods named something like `load` and `load_safe`.

For the example you gave, I think it’d be a much better design to have `fetch` and `fetchNoVerify` because this would make it much easier to audit the code to see if SSL verification is ever skipped.

This is the problem with blog articles showing minimum code examples. Minimum code examples are important because the more code you show the more your audience spaces out and moves on. The comments here are saying "but you just moved it to somewhere you didn't show", but it's the nuance in this comment that really gets to why this can be valuable.

> We likely don't care about all 8 states - rather 4 or 5, but complexity increases.

When you have only 2 valid use-cases, you need the code for both. It has to exist. When you have 5 valid use-cases and 3 invalid use-cases, making those 3 invalid use-cases impossible leads to better maintainability. So the minimum code example isn't actually the minimum code example to show why it is effective.

And yeah, product types and the ability to ensure that if you don't handle a case that is possible you get a compiler error is clearly the ultimate result the blog author is searching for.

I reckon the biggest problem for the web of doing things in the way this article calls type keys is that given current tooling it’s very hostile to optimisation, by making it harder to prove that a lot of the code is dead (and thus safe to remove).

A closely related issue is configurable libraries: far too many libraries become immensely configurable, but any one deployment is not using most of the code. (Give me a 10KB JavaScript library and the subset of its functionality used, and I can normally strip it down to maybe 2–3KB that will execute a good deal faster.)

What I’d really like for such cases is to be able to mark certain functions’ arguments as to be evaluated at compile-time—value monomorphisation, like type monomorphisation with generics. Or even mark arguments as allowed to be evaluated at compile time, so that the compiler can judge what’s optimal itself.

This is the sort of stuff that’s done as a matter of course in languages that compile to machine code (by compilers like GCC or LLVM), but it’s baffling how little effort has been put into anything like this at compile-time, given how much effort has been put into making execution fast. There are basically only three even slightly interesting things, and they all give you the choice between being extremely inferior, or being somewhat inferior and inconvenient:

• Google’s Closure Compiler’s advanced optimisations mode was good for its time, but hasn’t kept up (runtime tooling support in things like browser dev tools is nonexistent so that it’s painful to work with its output, and it needs TypeScript integration, and seriously, look at https://developers.google.com/closure/compiler/docs/api-tuto..., it gives a Python 2.4 snippet there).

• UglifyJS/UglifyES/Terser also live in the JavaScript era rather than the TypeScript era, so they miss huge opportunities, and their dead code detection and removal optimisations are pitiful by comparison with machine code compilers, being type-unaware and typically requiring inlining (regularly infeasible) before they might be able to do something. (Being type- and aliasing-unaware also thwarts a lot of practical code rearrange where a canny human can reorder things to shrink the resulting code and make it faster—again something regular compiled languages support, with Rust head and shoulders above the competition because of its ownership model.)

• Facebook did start the one interesting project in this area of the last decade, Prepack, but it’s fairly limited in its suitability because it’s too likely to break things unless you handle it with great care (similar to Closure Compiler’s advanced optimisations in that regard, and utterly unlike languages that are designed to be optimisable), and they seem to have given up on it now.

What’s wrong with subclassing and having default empty implementations of functions? I don’t understand what’s wrong with a default no-op lambda argument (except JavaScript or Java doesn’t seem to be able to provide one AFAIK).

Subclassing (and inheritance in general) makes it pretty difficult to follow the control flow as a given function call could resolve to one of many implementations, but there's nothing in the calling code to indicate which ones. Better to use composition and code which explicitly delgates to a sub-object where necessary.

How does using a sub-object make it any different? You can’t know which one is going to be called without knowing which object was put in the compositionary scenario exactly the same way as in order to know which implementation is going to be called you need to know the type of an object.

I don’t think your argument is strong here.

If you are calling a method on a delegated object then you know it's one of the delegated objects rather than a method on the base class. It makes it really easy to see which bits vary by between the delegated/sub objects and which bits are the same for all of them. With inheritance you have to check through every single sub-class to see if they've overridden a method.

Code paths where an overridden method in a subclass calls back into a method on the base class are much clearer too, as they have to be explicitly passed in as parameters, the sub-class doesn't have access to any of the parent class's state or methods by default.

Subclassing is not only about technique but also about semantics. Once you start with that, I will also be doubtful if the method "doX" or "createY" or "findZ" will do what they promise or behave completely unexpected.

That's why we try to follow liskov, and with your suggestion you are breaking this assumption/convention.

What if suddenly, after implementing, you have a new requirement of supporting users who are both a Customer and an Admin simultaneously? The resulting refactoring will be a lot more significant.

there is no way to organise code in such a way that it will equally cater to any change of the semantics down the line in an ergonomic fashion.

I have argued given the current puzzle, not additional “down the line” puzzle.

I don't see how that holds here. The author's proposed solution is composable, subclasses are not. Maintainability is an important property.

Subclassing the Usertype is a wrong solution here anyway. In fact this is a classic OO trap. Whether a user is an admin or not is a role of the user.

So a better class relationship is

User has a Role. Role can be an interface class with multiple implementations.

This design trivially lends itself for example to a use case where the user holds multiple roles. The classic OO design would use some form of "multiple" inheritance resulting in complicated class hierarchy.

It seems like the original was a silly way of doing it in the first place.

When you run into someone doing these things in the wild, you mentor them. When you make a blog out of doing things like this call it "Code Smell of the Day" you deserve the mockery.

Just wait until Jesse Duffield discovers ioctl.

This just seems like a poorer version of the strategy/replace conditional dispatch with command patterns.

I like type keys for exhaustive matching. I find it helpful to use the key to describe state, for example, then perform (or not perform) an action or show a component based on that state. To me it's idiomatic in that it expresses the different states and intent well - it allows people unfamiliar with the code to step in and read literally what the code is meant to do. I realize it's possible to get idiomatic results in other ways but I'm specifically drawn to how predictable and exhaustive it is. It removes a lot of guessing and unknown states.

I also like that in business logic or in presentation layers, wherever you go, various states are still consistently and clearly described. You end up with fairly cohesive types of functions which expect to do fairly specific things to specific shapes of data. To me this is a lot easier to manage than larger functions which do more based on a lot of conditions. The debugging experience is dramatically improved. Instead of debugging inside of if statements, you get to go upstream and check why something was assigned the wrong key in the first place.

One real-world example is search results in an app I maintain. The result types can either be "loading" (I know it'll exist but I don't have a response yet), "partial" (I got a response but some of the data could be missing or stale), or "complete" (this has all of the data I could ask for and I know it's fresh).

When a result is loading I don't want to render the final component which contains a lot of business logic that's dependent on a complete response from the server - shoving it in there and using a lot of conditions to avoid implementing that logic would create a complex component with very high surface area for bugs. Instead I keep a skeleton component which indicates that it's loading. It's actually the base for the partial and complete components, so I don't need to maintain it all that separately. Next, when a response is 'complete enough' (potentially stale or missing certain attributes) I render the partial component with slightly different logic and fields from the complete component. For a perfect result, I want to use its corresponding "complete" component.

So there's sort of a progressive enhancement happening that's very clearly described by the type keys and components, and I find it lets the code 'self-document' to a great degree. In terms of performance, I also know the UI will be re-rendering based on new data streaming in anyway, so rendering smaller and less complex components each time can actually be faster than relying on one big one.

Please, someone explain why this is a bad idea! I love to learn and I'm not married to this approach at all - I'm self taught and have tons of bad ideas.

What language is this?

Typescript for the user example, Java for the booking one.

Reply on news.ycombinator.com