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.<p>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.
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.<p>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…)
> Why Is This Solution Better?<p>It is not.<p>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.<p>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.<p>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.
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.<p>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.
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.:<p><pre><code> enum UserType {
ADMIN { void setup(User) { … } },
CUSTOMER { void setup(User) { … } },
;
abstract void setup(User);
}
</code></pre>
and then have:<p><pre><code> User createUser(UserType type, UserAttributes attributes) {
User user = new User(attributes);
type.setup(user);
user.setupNotifications();
return user;
}</code></pre>
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?
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.
This is smearing code around to distract from what is likely short-sighted IAM data architecture.<p>"Admins are Users except not really"<p>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.<p>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.
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.<p>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.<p>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.<p>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.<p>All of that causes a lot of runtime complexity and overhead for what is essentially static data.<p>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.<p>Example: A fictional function definition could look like this: (where # indicates a keyword destinied for the preprocessor)<p><pre><code> 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
}
}
</code></pre>
A compiler would generate two separate methods from this definition. (To avoid C macro madness, you'd probably <i>first</i> generate an AST, <i>then</i> have the preprocessor modify that AST.)<p>A call site could look like this:<p><pre><code> result = fetch("example.com/", {}, GET);
result = fetch("example.com/", {}, POST, data);
</code></pre>
Which would call one of the two methods in a fashion analogous to method overloading.
Looks like the author is 're-functionalising' (as opposed to <a href="https://en.wikipedia.org/wiki/Defunctionalization" rel="nofollow">https://en.wikipedia.org/wiki/Defunctionalization</a> ).<p>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.<p>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.
I wish my competitors will spend more time (ideally <i>all</i> 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 <i>improved</i> the productivity of the team (him not wasting everybody’s time with low priority nonsense).
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.<p>[1]: <a href="https://craftinginterpreters.com/representing-code.html#the-expression-problem" rel="nofollow">https://craftinginterpreters.com/representing-code.html#the-...</a>
Why not use something in the form of SingleDispatch on the typed key?<p>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.<p>Or, instead of singledispatch, use dictionary mapping `type(key)->Callable`, and pass the arguments there, making the graph identical to the second.
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.<p>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.
I reckon the biggest problem <i>for the web</i> of doing things in the way this article calls type keys is that <i>given current tooling</i> 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).<p>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.)<p>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 <i>allowed</i> to be evaluated at compile time, so that the compiler can judge what’s optimal itself.<p>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 <i>execution</i> 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:<p>• 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 <a href="https://developers.google.com/closure/compiler/docs/api-tutorial3#enable-api" rel="nofollow">https://developers.google.com/closure/compiler/docs/api-tuto...</a>, it gives a <i>Python 2.4</i> snippet there).<p>• 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 <i>might</i> 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.)<p>• 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 <i>designed</i> 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).
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.
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.<p>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.<p>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).<p>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.<p>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.<p>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.