7 ms·

It's not indirection, it's bad abstraction that's the real issue here. Consider the example used in the article: if x.startswith("foo"): do_something

• baddox 7y ago
  I agree with you. I think it becomes pretty clear when applied to the example in the article of code review. If the abstractions are designed and named well, why should the reviewer need to jump to the declaration of the function in the if statement? Assuming the checkHasPermissions function wasn’t added or modified in the PR, the reviewer should just consider whether it makes sense to do_something if and only if checkHasPermissions.
  • barrkel 7y ago
    You'd think so, but abstraction names are like comments: they are seldom updated when the implementation grows more hairs. In older code, or code worked on by a larger team, I always chase into functions to understand the code, and don't rely on names. I'm also less inclined to indirect code unless the abstraction has genuine meaning, rather than being a compression technique for repetition, no matter how well named.
    • cottsak 7y ago
      Then the problem here seems like the lack of drive towards intention-revealing naming. It's burdensome, but worth it when the intention is revealed and prevents the navigation to the implementation. But like the author said, the reuse fallacy prevents this from having a good RoI until there is a significant usage of said abstraction.
  • falsedan 7y ago
    > If the abstractions are designed and named well, why should the reviewer need to jump to the declaration of the function in the if statement? How will the reviewer know if the abstraction is well-designed or named without reading it? Code quality attitudes change over time so sometimes the standard project-provided helpers end up not matching the intuitions of more recent contributors.
    • baddox 7y ago
      It’s a valid question, but at some point in a code review you have to trust an abstraction layer, lest you end up reading semiconductor physics papers for every PR.
• catmanjan 7y ago
  Even better (IMO), "hasPermissions" It isn't clear whether the "check" prefix means the function returns based on whether the checking was successful or not.
  • jorangreef 7y ago
    I'll do you one better... if authorized(x): do_something_with(x)
    • deleted 7y ago
      [deleted]
    • psychoslave 7y ago
      At this point you might also consider a `try: treat(x); catch unauthorized: fallbackly_treat(x)` pattern.
      • leksak 7y ago
        Disagree. Someone attempting to do something while unauthorized is oftentimes not an exceptional state and regular business logic depending on how access is given to different endpoints. Do not use exceptions, which are "heavy" (capturing call-stack etcetera), for something that commonly occurs.
        • toxik 7y ago
          Put another way: exceptions are exceptions to the normal program flow. They should not be used to communicate expected errors. I think of them as analogous to the `if (do() != 0) { goto out; }` paradigm of C.
          • ChrisSD 7y ago
            Unless you're using Python where exceptions occur every time you finish looping. https://docs.python.org/3.7/library/exceptions.html#StopIteration https://docs.python.org/3.7/library/exceptions.html#StopIter...
• kstenerud 7y ago
  Actually, it would be a perfectly fine mini-encapsulation as-is. If you need to know whether something is foo-like or not, it's far better to have that test in one place rather than repeated all over the codebase. Otherwise debugging or modification would be a nightmare. Even from a code U/X perspective this is better. How is someone going to know that x.starts_with("foo") means that the object is foo-like, rather than some subset comparison only? The name "is_foolike()" tells them unambiguously what information the function will return and more importantly, what the information means. Abstraction is about assigning higher order meanings to groups of information and behaviors so that we humans can keep the concepts straight in our minds, and is_foolike(x) is a lot easier to understand the underlying meaning of than x.starts_with("foo"). How you react to something being foo-like or not is a question for a higher abstraction level to decide, and present a unified front from there, like get_foolike_items(y), which in turn is needed by get_high_value_targets("sweden") (because all high value targets have foo-like metadata), which is in turn passed to launch_nuclear_missiles(targets).
• isolli 7y ago
  Agreed. I remember it as: "show what the code does, not how it does it".
  • ElonMuskrat 7y ago
    That works great when the semantics of the code match the written intent. This is fine for battle tested libraries and pieces of infrastructure. However for code reviews and debugging an engineer must actually read and understand the 'how' to prove and disprove the 'what'.
• AstralStorm 7y ago
  Congratulations, this pattern is responsible for more security bugs than any other. Security should not be an "if" but must. Presumably down to instruction level or preferably to hardware that does TLB. This thing is a likely TOCTOU error. What happens if I pass the check and enter the function just not having the permissions? The only way this is acceptable is if the code is proven to never do that, preferably to the deepest level possible.
  • benj111 7y ago
    You're reading an awful lot into one 2 line throwaway example on a different topic. CheckHasPermission() could be permission to update the score on space invaders. Plus how are the other 2 examples in any way better from a security POV.
  • mav3rick 7y ago
    It was an example. It could very well have been checkForSomeNonCriticalThing(x).
    • AstralStorm 7y ago
      It's the principle of checking separated from using which is wrong in almost all cases. Instead you should cause and/or handle errors! (Also known in Python as duck typing when applied to types.)
  • cakoose 7y ago
    Like others said, not the point of the example. But even then, there's not enough context to determine if there's a TOCTOU bug. For example, what if it's using an RDBMS transaction?
• geocar 7y ago
  In many cases speaking of the foolike nature of a value is not dissimilar to speaking to the primeness of an integral: Is a value prime? And now here's is an algorithm for determining whether this value is prime. The motivation for many splitting out a prime-testing function is that primality testing is hard to do efficiently, not that there are many kinds of primes or many kinds of integrals. Indeed, in some languages "primes" is a builtin (p:), or a trivial derivation {(2=+⌿0=(⍳⍵)∘.|⍳⍵)/⍳⍵} for some domain, and that's the only domain our application is concerned with so is-prime is a (⊣∊primes) -- is foolike-ness like that? Nothing is being abstracted here, and nothing is gained by indirection: Does this is-prime routine even deserve a name? Sometimes the foolike nature has nothing to do with the value: Is an apple foolike? It isn't today, but it might be tomorrow! (That's numberwang!) Is this account locked (go check the disk!)? Sometimes we encode that the account is locked by putting an asterisk in the password field -- this won't match any hashes, so the login will always fail, and a "is_locked" routine might start exactly this way: def is_locked(x): return x.password.contains(42) but in the future we want to be sure that this isn't a _copy_ of a locked account or there's a synchronisation issue, so we go check and then def is_locked(x): return x.userid in locked_accounts Nothing is being abstracted here either- we're still only talking about giving this algorithm a name (or maybe delaying the discussion about which algorithm gets this name). So why don't we just say 'something in x.can instead of x.can('something')? Because the former means set membership and the second may be able to not reify the set. Proxies like __in__ might help, but it's not just the syntax: Our language has let us down (clearly, not enough laziness! time to haskell all the things!), and so we cheat: The locked button sets a flag on the user object, instead of adding a user to the real list of locked users. But what if our language were smart? Why wouldn't we write x.userid in locked_users? > Whereas the following code has meaning: And yet it is wrong! It has a nasty security bug in it that you will never find, and the next guy will never find. Fools and children would argue endlessly that something was wrong with the ticket or user story that led to this code, or demand that they didn't have enough experts on the team, but no matter how smart you are (or you think you are), you'll write a piece of code that is so short it cannot possibly be wrong and yet it is. Code hiding (whether you call it indirection or abstraction) also hides bugs, and yet few of us have the time or attention to stare at the bits blurry with a steady hand and a magnetised needle, so there'll always be some amount of code hiding. The real trick (if there is one) is to do as little of it as is possible.
• DonHopkins 7y ago
  What if the human reading the code is named "Ike" and he doesn't like being called a fool?
• jrochkind1 7y ago
  When the author used generic "foo" examples, it's hard to tell if there's meaning behind it or not. In a real example, there might very well be meaning behind it, perhaps his "foo" examples in his actual life are more like your "checkHasPermissions" example. But I think you're right that "abstraction" is the important concept here, which "indirection" is either a synonym for or one part of, depending on what you mean by "indirection". I think the main challenge of organizing your code ("architecture") is figuring out the right abstractions. And "too much abstraction" is definitely a thing. In my experience there aren't any magic rules or guidelines you can just follow to get the right level and kinds of abstraction. There are guidelines and things to think about, and while getting more experience writing (and maintaining!) code is the only way you get better at it, reading and thinking and discussing it helps too. Some things that have been attempts to guide people to the right levels and kinds of abstraction, like "design patterns", have proven to be no magic bullet either. In actual experience, I think most of us end up erring on the side of too much/too complicated abstraction ("over-engineering"), so when it doubt, it makes sense to try erring on the side of less abstraction, including duplicating code at times (do not let "DRY" be your only guide). Which, in the end, is what I think OP is telling us.
  • jschwartzi 7y ago
    > In actual experience, I think most of us end up erring on the side of too much/too complicated abstraction ("over-engineering"), so when it doubt, it makes sense to try erring on the side of less abstraction, including duplicating code at times (do not let "DRY" be your only guide). Yeah, this. I've noticed a lot of younger engineers(myself included when I was starting out) have a tendency to try to avoid duplication at all costs, even when it's unclear whether the two pieces of code will evolve in the same way over time. So you wind up with a single class but then a year later you have to rip the single class out and replace it with the repetition of the code, and then make a change that makes the two cases no longer repetitious. It would have probably been better to repeat the case. My heuristic for deciding whether to replace duplication with abstraction is whether I've copy-pasted the same code at least three times, and/or it is code where eliminating the abstraction is trivial. So if I can replace the code with a simple function I'm more apt to do it than if I would have to create a class or composition of classes to do the same thing that the duplication does. It's way more maintainable at that point to just leave the copy/paste in and deduplicate when there's actual need to create a new tool in the code base.
• dclowd9901 7y ago
  Great explanation, but I would just say `hasPermissions`. I like to name functions for their result, not for what they do.