1. 21
  1.  

  2. 16

    I like the point about type keys, but one thing that distracted me was the refactoring in step 3. Honestly, the code in step 2 was more readable. The different approaches in step 3 are an example of the DRY principle taken too far.

    1. 3

      I came to make the same comment.

      As a variant of Sandi Metz’s

      Duplication is far cheaper than the wrong abstraction

      I’ve recently come to:

      “A little duplication is better than one more abstraction.”

      The abstraction need not be wrong, or even artificial. It has a cost just by virtue of being an abstraction.

      1. 3

        createUserWithNotifications? I found that oddly specific as well.

        Admins may have different arguments for notifications so maybe you’ll end up encoding a kind of type flag in the call. Or encode a bunch of stuff in the names of four-line functions.

        The base premise is valuable but “overfitting the cleanliness” runs the risk of painting yourself in a corner and having to clean your way out again.

      2. 15

        I would much rather have a function with linear program flow from the top of the function to the bottom based on an if statement than a function that accepts a callback for non-linear flow, where flow jumps (at some point) out to a closure provided by the caller.

        At $DAYJOB, our programming style makes heavy use of discriminated union types (eg type Actor = { type: ‘user’, … } | { type: ‘bot’, … }) and exhaustive switch statements to handle variants. It’s very easy to add a new variant to a type, because the compiler will require you add support for the new variant to all the switch statements in every function that considers the type. Many of our core types have this .type ”key”. Does that mean our code is unhealthy? I think quite the opposite.

        1. 7

          This is one of those things like cyclomatic complexity limits that sounds like a great idea in blog form, but when turned into policy ends up simply making complicated logic harder to follow because it’s broken up for no good reason, polluting the namespace, and things become awkward when temporary or mutable (function-scoped) vars end up having to be passed to and from other functions, which in turn becomes even uglier when you’re working in a language without tuples or anonymous structs of some sort.

          1. 6

            This seems like a case where inheritance would be appropriate:

            class Customer extends BaseUser {
               override theMiddleSetupStep() {…}
            }
            
            1. 6

              It might be just the example but all I see here is a switch statement moved out from a convenience function into the caller. The type key is not “magically disappearing”, it’s just moved one level up.

              1. 4

                We are never shown the contents of the UserAttributes type, but I am suspicious that it contains (or would contain) fields which are expected to be null if the user is either an admin or a customer. Which suggests to me that a better way to model this would be with a discriminated/tagged union, which would look more like

                type User =
                  { type: “admin”;
                    adminAttributes: AdminAttributes;
                  } |
                  { type: “customer”;
                    customerAttributes: CustomerAttributes;
                  } 
                

                I’m also not really convinced that the type key “disappeared”—the code was just refactored so that instead of the high-level interface to user creation being one function which takes one of two values to distinguish the user type (ultimately two code paths), it is now two functions which take no “user type” arguments (still two code paths).

                1. 2

                  Yes, definitely agreed with regard to function arguments, but what you describe as type keys are also useful in other contexts, for example discriminated unions.

                  1. 1

                    Whenever two pieces of code need to change for different reasons, or at different rates, you must separate those pieces and minimise the dependencies between them.

                    I fail to see which two pieces of code need to change when the setup process for a customer changes.

                    1. 1

                      Sounds like a perfect use case for a Trait-based system