9 ms·

Code Smell of the Day: Type Keys

85 points by jesseduffield 5y ago

gregjor 5y ago
Describes control coupling with a different name, “type keys.”
vikingcaffiene 5y ago
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.
- xupybd 5y ago
  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 https://en.wikipedia.org/wiki/Dependency_inversion_principle [2] https://en.wikipedia.org/wiki/SOLID https://en.wikipedia.org/wiki/SOLID
  - theteapot 5y ago
    I can't see the Dependency Inversion ("Depend on the interface not the implementation") in this?
    - vikingcaffiene 5y ago
      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.
mastrsushi 5y ago
What language is this?
- cyber_kinetist 5y ago
  Typescript for the user example, Java for the booking one.
heavenlyblue 5y ago
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).
- nicoburns 5y ago
  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.
  - heavenlyblue 5y ago
    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.
    - nicoburns 5y ago
      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.
- 3np 5y ago
  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.
kgeist 5y ago
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?
- rightbyte 5y ago
  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.
  - layer8 5y ago
    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.
    - rightbyte 5y ago
      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.
stingraycharles 5y ago
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.
- diatone 5y ago
  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”
- biomene 5y ago
  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.
  - RHSeeger 5y ago
    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".
UK-Al05 5y ago
It seems like the original was a silly way of doing it in the first place.
shortercode 5y ago
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.
- eyelidlessness 5y ago
  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).
chrismorgan 5y ago
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-tutorial3#enable-api 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.
PartiallyTyped 5y ago
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.
- incrudible 5y ago
  Or, maybe the original code was actually totally fine and in fact the simplest and most obvious solution.
  - PartiallyTyped 5y ago
    Literals should be avoided since they can get mistyped unless the compiler can enforce that.
    - incrudible 5y ago
      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'
      - PartiallyTyped 5y ago
        I see. Then I am joining the club where the first one was perfectly fine.
      - beebeepka 5y ago
        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.
toxik 5y ago
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…)
- beebeepka 5y ago
  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
incrudible 5y ago
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.
- karatinversion 5y ago
  > 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.
  - incrudible 5y ago
    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.
  - gumby 5y ago
    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).
jbverschoor 5y ago
People have a weird understanding about what makes code understandable, readable and maintainable.
- tkiolp4 5y ago
  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.
  - only_as_i_fall 5y ago
    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.
    - withinboredom 5y ago
      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.
      - only_as_i_fall 5y ago
        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
    - 5y ago
nerdponx 5y ago
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.
- svrtknst 5y ago
  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)
  - nerdponx 5y ago
    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)` !
    - fiddlerwoaroof 5y ago
      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.
xg15 5y ago
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.
- wizzwizz4 5y ago
  Rust has this with its macro_rules! macros. It's not as good of a solution as it initially seems.
  - xg15 5y ago
    Interesting to know. What were the issues?
    - wizzwizz4 5y ago
      Mostly the syntax is bad, I think. Also, exposing the AST like this means implementation details about the parser are now locked in.
pech0rin 5y ago
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.
- marcosdumay 5y ago
  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.
tonetheman 5y ago
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.
layer8 5y ago
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; }
bob1029 5y ago
> 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.
kazinator 5y ago
Just wait until Jesse Duffield discovers ioctl.
pphysch 5y ago
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.
steve_adams_86 5y ago
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.
vips7L 5y ago
This just seems like a poorer version of the strategy/replace conditional dispatch with command patterns.
chriswarbo 5y ago
Looks like the author is 're-functionalising' (as opposed to https://en.wikipedia.org/wiki/Defunctionalization 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.
ctvo 5y ago
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.
mbrodersen 5y ago
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).
camgunz 5y ago
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-expression-problem https://craftinginterpreters.com/representing-code.html#the-...

85 points by jesseduffield 5y ago

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

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.

xupybd 5y ago
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 https://en.wikipedia.org/wiki/Dependency_inversion_principle [2] https://en.wikipedia.org/wiki/SOLID https://en.wikipedia.org/wiki/SOLID
- theteapot 5y ago
  I can't see the Dependency Inversion ("Depend on the interface not the implementation") in this?
  - vikingcaffiene 5y ago
    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.

mastrsushi 5y ago

What language is this?

cyber_kinetist 5y ago
Typescript for the user example, Java for the booking one.

heavenlyblue 5y ago

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

nicoburns 5y ago
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.
- heavenlyblue 5y ago
  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.
  - nicoburns 5y ago
    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.
3np 5y ago
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.

kgeist 5y ago

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?

rightbyte 5y ago
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.
- layer8 5y ago
  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.
  - rightbyte 5y ago
    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.

stingraycharles 5y ago

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.

diatone 5y ago
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”
biomene 5y ago
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.
- RHSeeger 5y ago
  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".

UK-Al05 5y ago

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

shortercode 5y ago

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.

eyelidlessness 5y ago
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).

chrismorgan 5y ago

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-tutorial3#enable-api 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.

PartiallyTyped 5y ago

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.

incrudible 5y ago
Or, maybe the original code was actually totally fine and in fact the simplest and most obvious solution.
- PartiallyTyped 5y ago
  Literals should be avoided since they can get mistyped unless the compiler can enforce that.
  - incrudible 5y ago
    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'
    - PartiallyTyped 5y ago
      I see. Then I am joining the club where the first one was perfectly fine.
    - beebeepka 5y ago
      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.

toxik 5y ago

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

beebeepka 5y ago
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

incrudible 5y ago

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.

karatinversion 5y ago
> 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.
- incrudible 5y ago
  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.
- gumby 5y ago
  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).

jbverschoor 5y ago

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

tkiolp4 5y ago
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.
- only_as_i_fall 5y ago
  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.
  - withinboredom 5y ago
    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.
    - only_as_i_fall 5y ago
      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
  - 5y ago

nerdponx 5y ago

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.

svrtknst 5y ago
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)
- nerdponx 5y ago
  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)` !
  - fiddlerwoaroof 5y ago
    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.

xg15 5y ago

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.

wizzwizz4 5y ago
Rust has this with its macro_rules! macros. It's not as good of a solution as it initially seems.
- xg15 5y ago
  Interesting to know. What were the issues?
  - wizzwizz4 5y ago
    Mostly the syntax is bad, I think. Also, exposing the AST like this means implementation details about the parser are now locked in.

pech0rin 5y ago

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.

marcosdumay 5y ago
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.

tonetheman 5y ago

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.

layer8 5y ago

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

bob1029 5y ago

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

kazinator 5y ago

Just wait until Jesse Duffield discovers ioctl.

pphysch 5y ago

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.

steve_adams_86 5y ago

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.

vips7L 5y ago

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

chriswarbo 5y ago

Looks like the author is 're-functionalising' (as opposed to https://en.wikipedia.org/wiki/Defunctionalization 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.

ctvo 5y ago

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.

mbrodersen 5y ago

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

camgunz 5y ago

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-expression-problem https://craftinginterpreters.com/representing-code.html#the-...