Avoid Indirection in Code

    if x.startswith("foo"):
      do_something_with(x)

    if is_foolike(x):
      do_something_with(x)

    if checkHasPermissions(x):
      do_something_with(x)

Remember although code is interpreted by machines it's read by humans...

I understand the arguments for 'Uncle Bobifying' code but I think, as the article says, there's a balance to be struck. It's highly likely the next time I see your code (or my code if I'm coming back to it a month or two later) is when I need to fix a problem with it. While it's useful to have it split up into logically discrete chunks with sensible naming these 2-4 line functions far increase the mental load of the code.

Given I'm debugging the code because there is presumably a problem with it I can't trust these sensibly named functions to do what they say, I need to check their internals to reason about changes in application state. Though well named functions at least help the maintenance programmer reason about the why they're equally liable to code rot as the comments they replace.

As in all things there's a balance to be struck but I'd rather see a 20-30 line method than jump between 5 files because someone has read Clean Code recently.

Zed Shaw puts it well: "Abstraction and indirection are very different yet cooperating concepts with two completely different purposes in software development. Abstraction is used to reduce complexity. Indirection is used to reduce coupling or dependence."

I call it onion skin development, where the developer keeps hiding stuff in more layers of the onion, making my eyes water as I have to dig deeper and deeper to essentially find `a.foo(b)` under 12 layers of abstraction.

They're so focussed on making everything look so purrty, they forgot that it's about telling the computer to do something, as clearly as possible.

    if (url.startsWith('http://')) {

    if (isAbsoluteUrl(url)) {

It might make sense if you're trying to do a comparator, like "is_email_equivalent" - in that case the indirection would make sense - you might be doing a just a lower case check today, but in the future you might expand that to take into account Gmail specific equivalence rules.

For stuff like "read_json_from_file", indirection makes a lot of sense. No point doing the whole "create_buffer", "read_file_to_buffer", "parse_json" deal every single time.

Secondly, and in contrast to the point of the article, it can usually help readability.

If you're wrapping standard library functions, then I agree, the examples in the article are bad candidates. If you're dealing with something where magic values are involved, for example, then something like the following can greatly help readability:

    /* Checks if a given user id was generated by the legacy system. These are either 5 digits numeric, or start with "B" or "L". */
    is_legacy_user(userid)

Put together, the code flow is easier to follow, and if you're in doubt about the method you can check out the unit tests to see what situations exist that might not be obvious at first glance.

Edit: actually, I take that back. Even if you're just wrapping a standard library call, then there's really no issue so long as there's a domain reason (eg, is_legacy_userid() vs is_userid_fivedigits()), and there's unit tests. The unit tests can prove the standard library function is all that's needed.

A Philosophy of Software Engineering:

https://www.amazon.com/Philosophy-Software-Design-John-Ouste...

This is mentioned as one of the symptoms/appearance of bad code: Shallow/Passthrough methods, which results deep call chains, adding up cognitive overload. The author in this book recommends deep module over shallow module, which tends to end up with more cohesive code.

There is a related problem that causes more trouble, but it cannot really be blamed on the initial abstraction. It goes like this: you have something, such as a foo-ness test, that is needed in several places, so you abstract it as a function for all the right reasons. Later on, one of those uses changes, and needs slightly different logic than the other cases. Instead of writing a new function (that may, as part of its implementation, call the original), the person making this change (not you, of course!) modifies the original function to handle both cases. This, by itself, makes the code more complex than necessary, and it gets worse if the criterion for which path is taken in the function is not something that has any significance for someone who arrived at this function through reading the code for one of the other cases that call it. And if the new path has a side effect, now all the other cases are also calling a function that potentially has side effects...

This is how code rots.

Visual Studio has this and it's called "Peek at function".

From the article it seems like the bigger issue might be debugging style. For me, when debugging those abstracted functions should be treated and trusted as black boxes until you have sufficient reason that function is the issue, and then dig into it and ignore the rest of the function. Trying to build a mental model by effectively flattening every function takes huge cognitive load, and only is feasible for small amounts of code. Once the codebase is large enough it's going to become impossible to do this.

Abstraction is the key. Abstraction can be used to reduce incidental complexity by hiding an implementation. Abstraction can be used to reduce the cognitive load of the essential complexity by making the code look like the problem domain.

But there is a cost to abstraction. A person reading your code may not have the same mental model of the problem, and might’ve be working at a higher or lower level of abstraction, in which case the "indirection" might be a stumbling block. Weigh these costs carefully.

Abstraction and indirection are very different yet cooperating concepts with two completely different purposes in software development. Abstraction is used to reduce complexity. Indirection is used to reduce coupling or dependence.

https://zedshaw.com/archive/indirection-is-not-abstraction/

By substituting a series of lower level concepts/tasks/procedures into a single keyword the reader can more easily think about the problem domain. That's a good thing. If they know the abstraction, it reduces the number of things the reader must keep in their head.

However, if the reader is not familiar with the abstraction they will need to spend time to familiarize themselves. This can break down when trying to understand an abstraction leads to the "endless rabbit hole" problem. Basically, you have to open another file to see how that abstraction works, then it uses some other abstraction which you need to open another file to read, and so on. If the depth is too great the reader will lose context and won't be able to fit the problem in their head.

To avoid this, pull more of the functionality into the function instead of outsourcing it to an abstraction. Favor writing code that reads sequentially rather than as a series of jumps through a bunch of files.

As always, this is largely a matter of taste so use your best judgement and balance abstraction with pragmatism.

> x.start_with("foo")

do

> x.length >= 3 && x[0] == "f" && x[1] == "o" && x[2] == "o"

Now the reviewer doesn't have to jump to the definition of "starts_with." Isn't that way more readable?

If you repeat this a lot, you might have a domain concept. If you can come up with a name that makes sense for your domain then you should probably take that domain concept and realize it within your code in the form of some kind of abstraction - e.g. a function call. Even if it is as simple as a .startswith call.

If you can't come up with a name, kinda depends upon how prevalent it is. If you just do it a handful of times, no big deal.

First, if you're defining interface indirection to support encapsulation and you wind up never rewriting (or adding another) implementation behind that "external function" or method, you probably didn't need it. (YAGNI, yo?)

Second, if you're defining interface indirection and it makes it harder to understand the code, you're doing it wrong anyway.

- - - -

Indirection that hurts is like when you do dispatch through a dict of functions and now all your tools, debugger, etc. can't tell you about the static structure of your code:

    dispatch = {
        'name': handler_func,
        ...
    }

    ...

    dispatch[selector](*foo, **bar)

> "Everyone knows that debugging is twice as hard as writing a program in the first place. So if you're as clever as you can be when you write it, how will you ever debug it?"

–Brian Kernighan, "The Elements of Programming Style", 2nd ed., chapter 2

(Also, notice that, in Python at least, derp.baf(foo, *bar) is the same thing, eh? EH??)

With functional languages the mutable data is available in the reader's context. The implementation might still be mysterious but hopefully the function name is good enough to give a clue about that.

For example, before the transformation:

    function isFooLike(x) {
        return x.startsWith("foo");
    }

    function isBarLike(x) {
        return x.startsWith("bar");
    }

    function bigFunction(str) {
        if(isFooLike(str)) {
            console.log("Foo!");
        }
        else if(isBarLike(str)) {
            console.log("Bar!");
        }
        else {
            console.log("Unknown!");
        }
    }

    function bigFunction(str) {
        if((function(x) {
            return x.startsWith("foo")
        })(str)) {
            console.log("Foo!");
        }
        else if((function(x) {
            return x.startsWith("bar")
        })(str)) {
            console.log("Bar!");
        }
        else {
            console.log("Unknown!");
        }
    }

    "We can solve any problem by introducing an extra level of indirection" 

[0] https://en.wikipedia.org/wiki/Indirection

[1] https://en.wikipedia.org/wiki/Fundamental_theorem_of_softwar...

  if is_json(somestring) {
      // do something
  }

  function is_json(string s) {
      return JsonLib.convert(s) != NULL
  }

The given example x.startswith("foo") is simple enough to include it directly. However, if there is a business reason for why "foo" is checked here, it will be difficult to understand why the check is there. This will be more difficult to read a code like this:

  if x.startwith("-")
      do something

  if is_yaml_node(x)
      do something

  def is_yaml_node(x)
      x.startwith("-")

> Would this code be easier to read if A) I inlined the code or B) I replaced it with a descriptive function name.

Function could be read as Class or Method if you're dealing with OOP.

One important criterion for this is that other programmers already know the language, not your program. Writing your own function can help to put a meaningful name on a set of behaviors, but it also means the introduction of a new specific set of behaviors the reader needs to keep in the back of their mind. It's kind of similar to Jakob's Law [1] in a way, but applied to source code. (In a way, source code readability in't all that different from UX, if you think about it)

[1] https://lawsofux.com/jakobs-law

An article like this at least sets some precedent and gives you something to point to.

You understand what 'has_kettle_boiled' means.

Inlining functions makes it difficult in a sequence to work out what are the separate steps. Splitting those out into a function - particularly if they take the temporary variables with them - makes it easier to understand what the higher level function does, not harder.

Perhaps we should revisit stepwise refinement as a technique and the need for functions to be loosely coupled and highly cohesive.

Just last week I was handed a project with single functionality: 'Doing ldap bind on receiving userid/password over http and return success/failure'. This project has about 40 Java files in 18 directories. As far as enterprise projects go it follows all best practices of Java and Micro services etc. But I find this project absolute turd and hopeless to refactor. A clean rewrite would be only sane thing.

https://web.archive.org/web/20130227000541/http://defect.ope... https://github.com/oracle/solaris-ips/commit/53bb14f750dd111...

I mean, who reads code linearly anyway. My hand hovers over F12 (the shortcut for go to definition in Visual Studio) half the day. Any good IDE will help with this.

I feel like this person is like, I don't want to drill all these holes to build my deck. I'll use nails. And it's like, "dude, somebody invented the 3 inch self tapping deck screw; go to town."

I think this is the problem right here. As a new reader you should first trust the function name does what it says it does and continue linearly. Later on, if you need to go back and look at the details or where the function name misleads or does more than it says.

I find this code hard to work with since without actually debugging the code there is no easy way to just click on a function and see what is called - a lot of functions reference a data structure of I2C function pointers...

1. It takes a lot of time.

2. You might end up with bad abstration that in the end hinders you. You then have to work around the abstration you created.

3. More functions create more entries you need to understand in your mind, this can sometimes make code harder to reason about.

DRY is a awful "thing" (a decree at best). In the worst interpretation it simply says "never write the same code twice". There is no balance or end to it. It doesn't have a competitor or alternative. It somewhat implies that it is always good. It doesn't define a scope where/when it should be applied. There is simply no broad agreement how to use it in practice. DRY touches multiple concepts, each too complex to put behind three tiny letters.

OP fails to make a great point, but the direction is right. We cannot take DRY and "code reuse" laws as granted. Abstractions and indirections have their downsides. It increases systemic complexity and may add dependencies. It certainly limits how easy the full system can be understood by humans.

Looking for a non-contrived and/or non-scarecrow example? Anything in the Chef code-base circa 2014 or prior.. No clue if it has changed.

Ruby code bases were the poster children of DRY-gone-wild.

Some of the code ran remotely using RPC, so part of the code sat in different repositories.

Naturally this called for multiple levels of abstraction and indirection. The results ? a big ugly pile of levels if you are trying to understand what the code does, that led to bugs, unnecessary complexity since it's hard to foresee the future and always abstract the way the code will need and the worst was that people simply gave up on trying to contribute do to the complexity of the otherwise simple code.

But in relation to the actual problem of legibility to a new developer still building up the mental model of the code, this is an area where tooling can help. You ought to be able to view the called-method’s body with a keystroke, without losing the current context, in an IDE or a code review tool. I’m sure some IDEs do have such features. Yes this is harder in some languages than others but in general it’s too bad these sorts of features aren’t built in to Github and Gerrit as well.

Abstract aspects likely to change, via interfaces that aren't, says Parnas (On the Criteria To Be Used in Decomposing Systems into Modules, https://www.win.tue.nl/~wstomv/edu/2ip30/references/criteria...)

abstract-unless-leaky: hide information you don't need.

In practice, writing tests helps me see what is naturally part of a module vs. a usage of it. You want the module to contain everything it needs to do its job, and nothing else. But this assumes the modularization (its "job").

“Abstractions — or rather, indirections — are bad for for readability, but they are good in some cases.”

Umm... this can be said for pretty much anything. Over use of anything is bad. Inappropriate use of anything is bad.

> Mostly, I want authors to be aware that there is a human cost to indirection that is felt more acutely by everyone reading the code except the original author.

Sure. It’s a well known fact that abstractions have associated costs — nothing new there. However, the author seems to forget that not abstracting things can be equally costly — even when human readability is concerned.

Imagine someone trying to figure our the business logic be reading code. All they need to know is some conditions are being met for certain steps to be executed. The actual steps involved in the check are irrelevant to the business logic. The condition can be modelled separately from the business logic and can even change independently. For example:

    def is_foo_condition_satisfied((self):
        return self.variable.startswith(‘foo’)

The most important thing to note is — programming is not confined to s strict set of rules. It’s not an exact science. You can’t define a strict a rules that must be followed. What makes sense somewhere is completely useless elsewhere. The Linux Kernel is full of goto statements, despite the fact that goto is considered harmful.

Dogma Driven Programming must be avoided. Even well known programming practices are guidelines at best. One must evaluate whether a given rule fits a given situation.

Coming back to the example at hand, functions are meant to divide the program by the logical tasks being performed, not by number of lines or any other metric. As I said before, it’s more art than exact science.

Is the check for foolikeness of something a logically distinct task? It depends on the context. There is no way to write a cardinal rule either way.

PS. I sincerely hope no one comes to me for code review with a monolithic function because of all the inlining — citing this article as inspiration.

Edit: Typo correction.

I think peek definition could help you out when you are really dealing with it in the code base. I just wish it would be smarter. Peeking a function with 5 lines, peek it. If it’s larger, jump to it.

From my limited experience, I think abstractions like these are helpful as long as there is consistency.

This is easy to do by yourself but not always on a team; however, I think Rails has a good balance of conventions that allow indirection to be successful as long as the people writing the code use the same patterns and communicate effectively.

Today, I try to do big functions, multiple returns and const where I can. I still use code-blocks sometimes to scope variables.

I sometimes end up with a few hundred lines of code per function, but I can usally scroll over it and find what I need without jumping around.

Especially in JavaScript the addition of const and async/await helped much with this.

Sometimes we really do need a one-line function to implement a common interface. Sometimes we need it so that, in the two places it's called, both get the new behavior when it changes.

But abstraction for abstraction's sake is just stalling. The technical term is "ratiocination". It's a filthy habit.

Sure it is some effort to backtrack to the util functions, but I think the effort is worth it because I would rather have a developer spend multiple hours debugging and figuring out the code than having something that is a nightmare to iterate upon.

The name of a function should abstract away from "how it does something" not "what it does".

If I can't easily see the contents of is_foolike, should I stop writing my code like this or should the editor be showing me these details more easily?

The real problem: it takes me too much time to see what's inside the function.

In any other case the whole code should be in one file.

I dont understand why there isn't a code editor that will inline functions for you?

The server at www.matthewrocklin.com is taking too long to respond.

too many HN readers perhaps?

If I was doing a code review and came to this:

    def is_foolike(x):
        return x.startswith("foo")

This addresses both concerns mentioned in the article:

1. During review, when a reviewer is asked to verify that code is sensible before it can be merged into the main project. That reviewer probably has about a tenth as much time to spend as the original author does on that code. But if the reviewer comes to the code

    if starts_with_foo(x):
        do_something_with(x)

2. While debugging future issues. This code will eventually be involved in a bug and some completely different developer will have to glance at this code to figure out what’s going on. They’ll have to understand some small section this code within a few minutes to determine what is relevant. They won’t be able to invest the time to understand the full thought process behind it, and a web of function definitions can slow down this process considerably. But again when they read the code, a good name means they can understand what the code does without having to read the implementation.

I do think there's an argument to be made that indirection is a problem here, but it's a small problem compared to the enormous problem that `is_foolike` is a really bad name for that function.

General rules for when to NOT pull something out into a function:

1. If it the function call wouldn't be clearer than the code (even a better name like `starts_with_foo(x)` loses some information contained in `x.startswith('foo')`, and the latter is readable enough that there's no real upside to the former. If the latter were even two lines long, it would become a lot more worth it.

2. If there's no repetition. Two similar pieces of code aren't enough: you don't understand from two use cases what pattern you're abstracting, so the result is just going to be a leaky abstraction. Three repeated pieces of code seems to be the sweet spot: now you have enough examples to know what's actually repeated, what should be arguments to the function, etc.[1]

[1] https://blog.codinghorror.com/rule-of-three/

Yes. Please. Stop. Doing. That.