1. 12
  1.  

  2. 25

    A few days ago someone tweeted a question asking which of the following PHP snippets was better than the others, or whether there might be an even better approach . . .

    I tweeted my answer:

    Place the if/else cases in a factory object that creates a polymorphic object for each variant. Create the factory in ‘main’ and pass it into your app.

    Is this parody?

    edit:

    by Robert C. Martin (Uncle Bob)

    Oh. Right.

    1. 11

      Is this parody?

      I also initially did a double-take when I read that, but I wonder whether you carefully read what follows. His assumption is that much more depends on the 0/1 here and the if/else (or case) would metastatize across the codebase if no measures were taken. Having not seen any context for the original question, that could be a valid assumption.

      In general though, it feels like premature abstraction. And the wrong kind of abstraction is more costly that not being DRY (paraphrasing Sandy Metz (I think))

      1. 5

        What he says is nothing crazy.

        Having the Base class delegate functionality on its subclasses is something pretty standard in Smalltalk and is a nice way of simplifying code following the SOLID principle.

        1. 2

          It’s not crazy just more complex than is probably needed.

        2. 4

          Is this parody?

          I hope so,

          edit:

          by Robert C. Martin (Uncle Bob)

          Oh. Right.

          I don’t know whether this proves it one way or the other.

          1. 10

            I don’t know whether this proves it one way or the other.

            It’s definitely parody, but without any of the self-awareness that would normally lend it humor.

          2. 3

            Why would it be parody? In the context of the whole article it is clearly not. The snippet you chose to quote would be by itself, of course, but that’s why the rest of the article is there…

          3. 9

            Place the if/else cases in a factory object that creates a polymorphic object for each variant. Create the factory in ‘main’ and pass it into your app. That will ensure that the if/else chain occurs only once.

            […]

            My fear is that the if/else/switch chain that the author was asking about is replicated in many more places within the code. Some of those if/else/switch statements might switch on the integer, and others might switch on the string. It’s not inconceivable that you’d find a if/else/switch that used an integer in one case and a string in the next!

            This seems like a weird argument to be honest; the entire point of that determineGender() function is to put the logic in a single place, so it doesn’t need to be repeated.

            Other higher level modules tend to depend on the modules that contains those if/else/switch statements. Those higher level modules, therefore, have transitive dependencies upon the lower level modules. This turns the if/else/switch statements into dependency magnets that reach across large swathes of the system source code, binding the system into a tight monolithic architecture without a flexible component structure.

            So? I don’t really see the problem with this, certainly not for this use case.

            1. 6

              I don’t know PHP but in many languages, I would refactor this to a table lookup, something like:

                 genders = [ 'male', 'female' ];
                 return genders[input] || 'unknown'
              

              Table lookups are often overlooked when considering how to implement something but they’re often much faster, cleaner and more succinct than alternatives.

              The out-of-range/error handling would vary greatly by language. Rust has .unwrap_or(), C would need an explicit if condition at which point this won’t look especially elegant. In many applications, the out-of-range condition would be an assertion or error rather than an “unknown” value. I appreciate that my comment is not really considering anything beyond the initial code snippet in the original article and is not applicable to the later parts.

              1. 1

                For very small tables (IIRC <=8 or so, last time I tested it) these kind of lookups usually aren’t really faster vs. looping over an array/list, and 2 comparisons are faster still. I assume this is due to the overhead of hashing the keys.

                Array lookups like your example would perform a bit better, but I wouldn’t be so sure if they’re faster than a simple ===.

                Not that it really matters in >99.9% of the cases.

                1. 2

                  Not faster: true. But clearer.

                  1. 3

                    Yeah, I agree. I would write it like the above as well actually, unless there’s a compelling reason not to. One of the nice things is that you get an iterable array (or map) of all values for free, which is often useful in various cases (like validation for example, or listing all options).

              2. 5

                It’s true that to add a new case you need to alter the switch statement. It’s still a very small thing to do, and it’s much simpler and easier to understand than the absurd inversion-of-control approach the author proposed.

                On the other hand, in languages with type classes you would be able to modify the branching logic without touching the original piece of code.

                1. 2

                  Curious why you would consider ad-hoc polymorphism an acceptable solution but scoff at class-based OOP polymorphism as a solution?

                  1. 1

                    It depends on n. If n is large class-based subtype polymorphism is a good solution, but for simple situations like these… I don’t know. I think Martin is generalizing here, but the example in the article is not the best one.

                2. 5

                  Since this article is analyzing and comparing example code, I think it would’ve been nice if there was sample code for the proposed implementation.

                  1. 4

                    I can kind of see the point, but bringing it up with such a dumb example as this one doesn’t make it very easy to understand. The example function is input validation logic that would normally sit at the edge of your system, so sentences like:

                    Some of those if/else/switch statements might switch on the integer, and others might switch on the string.

                    Just feel puzzling, at least to me. If the input is an enum, Uncle Bob’s fear is that this if/else chain, or switch, would be replicated in many other parts of the code to drive bits of business logic:

                    type payment_method = Card | Cash
                    
                    let pay = function
                      | Card -> pay_by_card ()
                      | Cash -> pay_by_cash ()
                    
                    let frob = function
                      | Card -> do_this ()
                      | Cash -> do_that ()
                    

                    …and so on, which means that if you want to add another payment method to that enum you’re gonna have to touch 10 functions rather than one, or if you want to change the behavior of the Card case based on some external factors (maybe you just want to test without calling the actual API? maybe turn it off at runtime?) the code is gonna get pretty ugly.

                    Which is probably a fair complaint to make about some systems, but I don’t think this war against if/else that people like Uncle Bob are perpetrating is gonna change anybody’s minds if they make it look like the default action when presented with code as simple as this is to turn it into a complex factory and derived classes mess. The fact that it involves a loaded topic like gender doesn’t help it score any points either.

                    1. 4

                      if you want to add another payment method to that enum you’re gonna have to touch 10 functions rather than one, or if you want to change the behavior of the Card case based on some external factors (maybe you just want to test without calling the actual API? maybe turn it off at runtime?) the code is gonna get pretty ugly.

                      It will be ugly regardless, as this is essential complexity. In general, this is the expression problem.

                      1. 1

                        Of course it’s a tradeoff, it depends on whether you’re changing the interface more often than you change the individual cases’ implementations. Which is why the notion that you should always solve this problem by subclassing, like this post says, is misguided.

                    2. 4

                      Stepping back a moment from the narrow-minded insistence on objects, we should consider what the question is actually about.

                      First case: this is a question about the clarity of code and the options for expressing the thoughts of the programmer. Considerations should include correctness, ease of quick comprehension, ease of expansion to new inputs, and theoretical performance. It’s unlikely that the input space would remain limited, so a case switch is obviously superior for all of these factors except performance.

                      Second case: this is a question about parsing an input to get a string which will be used for display purposes. The same considerations apply as in the first class, but we should consider one more: consistency in our codebase. Is there already a convention of how this is handled elsewhere? We should either do the same thing, or re-do the others if we are convinced that a particular approach is superior. Case switches are clearly more readable for multi-valued inputs.

                      Third case: this is a question about a data structure to represent gender. I’m pretty sure this is deserving of an analysis at least as sophisticated as Obergefell v. Hodges: the database engineering perspective and I can’t do it justice here. There are at least three defensible positions: (a) gender is not actually germane, so this input should just be dropped; (b) gender needs to be represented but the program makes no decisions based on it, so a string is exactly right; (c) the program is going to make decisions which involve gender, and so a string is a terrible choice – we’re going to need an easily extended enumeration that maps into our data storage, with a single lookup table and UI features that make it easy for appropriately permissioned users to see the range of known values and add new ones. This is a major legal and ethical minefield that strikes at the heart of personal identity. Anyway, we’re going to need a case switch.

                      1. 2

                        Assuming and OOP environment (obviously in a functional context we would use different words) your b and c are exactly what the article says. If you really just want to make this number a string, use what your syntax makes cioncise and obvious, otherwise you want objects to represent these things so that you don’t have to “make decisions” by using yet another if after this point.

                      2. 3

                        I think we’re meant to assume that this codebase is large and in need of overt detangling. I’m sympathetic to that. But this is a function taking a native integer and returning a native string. What dependencies? It looks to me like parsing logic, the kind you want to do ASAP after input and then handle the resulting data, and whose logic you may not even share outside the module that receives the input. A private function sounds great.

                        Uncle Bob explored the premise that the data in question might be used for policy decisions and arrived at a low level factory to create a polymorphic object to hide the switching logic. So for that high level policy to be enacted, you either have to spread it down to the low level too, or you’re going to switch again on the result, just like the original function. I may be missing his point.

                        1. 1

                          You would neber switch on an object value. To add some policy based on the result you simply add a method to the relevant objects and then invoke from your entrypoint.

                          1. 2

                            I follow, but adding a method to those objects is just what I mean by spreading high level policy logic into a low level dependency. Uniting policy with parsing seems like a very tall, thin slice to separate from the rest of the system.

                            I’m also not suggesting you switch on a polymorphic object pointer. I mean if you did not manifest the high level policy down in that polymorphic call, you would have to do it someplace else after receiving whatever the object was scoped to tell you. That permits the policy to stay high level but doesn’t result in much of a reason to use a family of objects over a plain function.

                        2. 2

                          The cult of objects and abstractions and patterns everywhere.

                          All you need is an enum like type and a function to convert input to that type. Maybe in some insanely more complicated example you could justify the factories and objects and visitors and what not but this code the question was asked about can be made type safe fool proof and no repetitive with a basic type and a function.

                          He advises against an enum in the article. I don’t really understand why. Maybe it is too simple. Too pragmatic.

                          1. 1

                            He says why right in the article. The assumption is that you will be conditioning real behaviournon this, probably in many places in the system. If you’re not, you don’t even need the enum and a string will do, if you are then the enum will lead to repeated cases all over the system vs a set of objects encapsulsting that complexity.

                          2. 1

                            It’s breaking things down into cases, all the way down. Behavior-first, 1 function switches over N cases. Variant-first, N classes each describe their own 1 variant of the fn. I like behavior-first. I don’t mind writing switch statements. Exhaustive pattern matching over ADTs, even better. Preferring to do your case analysis through OO virtual dispatch is odd to me.