9 ms·

Documenting code via single-caller functions is usually a mistake, because to everyone else who looks, the set of functions is an API. Both internal and extern

Documenting code via single-caller functions is usually a mistake, because to everyone else who looks, the set of functions is an API. Both internal and external APIs must be kept coherent. Also when readers are trying to understand exactly how some function changes the system state, having to refer to numerous other functions it calls is tedious.

OJFord 7y ago
> Documenting code via single-caller functions is usually a mistake, because to everyone else who looks, the set of functions is an API. That's a really good way of putting it. I try to avoid it by not writing new functions, but gladly using existing ones, in my implementation of whatever single new one. For example if I'm writing a find_and_update_foobar function, I'll use find_foobar if it exists, and with the right signature, but I won't write it just to implement the one I actually care about; ditto update_foobar. But, professionally I've mainly only used python; so I find it still deteriorates into a mess. (I type hint extensively, but still it only takes some missing hints, or something too loosely - or wrongly - typed.) I haven't used rust professionally/enough/on something large enough to be sure, but my feeling is that it just having a type checker prevents so much mis-refactoring.
- nicoburns 7y ago
  Interestingly, Rust can be quite opinionated abour code organisation (it makes you keep things in bigger chunks), because some of the compiler analysis doesn't work through function boundaries.
  - heavenlyblue 7y ago
    I think this one is bullshit, can anyone else comment on this one? Are there things you may only do iff you using a single function body?
    - nicoburns 7y ago
      A key one is partial borrows of structs (i.e. borrowing only 1 field, and then later borrowing a different one).
- zelphirkalt 7y ago
  But then what happens to "Your function should be doing one thing and one thing only." (The thing indicated by its name, considering the current abstraction level.)? Perhaps it is not always a bad idea to decompose that function into 2 properly named "steps", which reside in their own functions.
cma 7y ago
> Also when readers are trying to understand exactly how some function changes the system state, having to refer to numerous other functions it calls is tedious. This also applies for small inlined common helper functions. If it folds two or three operations into one, but doesn't have an obvious universally recognizable name, it would often be way clearer to a reader of the code to just put the few operations in directly.
- papito 7y ago
  Because we write code for other PEOPLE, not the compiler.
  - cma 7y ago
    If it is inlined it is usually the same thing to the compiler either way in my example.
marcosdumay 7y ago
> Also when readers are trying to understand exactly how some function changes the system state Yep. That's why changing system state is better kept at a minimum, and at the highest layer possible. Most languages have a very clear public/private marker for determining what is an API and what is just there for convenience (it doesn't have to be literally public/private, for some it's scope, others just tag the names, etc). You don't need to change a code's style just because of it. On the other hand, if the functions do not have logical and atomic meanings, they will do make your code as hard to understand as it will be to name them.
notJim 7y ago
This is such a good, succinct way of putting it. One of the worst code-bases I've ever had to work with was largely so because of so many single-caller functions and far too granular abstraction.
- threatofrain 7y ago
  It seems that's how a few major libraries are organized, sometimes one function per file.
emsy 7y ago
Since this comment finds a lot of agreement I'm going to play the devils advocate. Private function for the sake of documentation are fine. At least if they are all on the same level of abstraction. If you've ever worked in a team where everyone works this way you stop thinking in APIs, which can be a good thing for internal code.
BurningFrog 7y ago
> Documenting code via single-caller functions is usually a mistake, because to everyone else who looks, the set of functions is an API. Not when you work on a team that does this as a practice. Many teams don't manage to have a common code style, which is a big problem in itself. > Also when readers are trying to understand exactly how some function changes the system state, having to refer to numerous other functions it calls is tedious. The point of breaking out the small functions is to name them so understanding the code becomes easier. This takes some skill and thought, of course, and if done thoughtlessly, it will not be good. But that's true of every practice...
- barrkel 7y ago
  It especially doesn't work in a team, because under maintenance + employee turnover, these little single-purpose functions start growing hairs, and they end up doing more than they're advertised to by name. Next thing you know, you're fetching the same thing from the database in three different functions, or recalculating the same value repeatedly. Overly granular functions constructed to name blocks of code are a firm anti-pattern in my book.
  - emsy 7y ago
    Functions overbearing their purpose can be done in large functions too, arguably even easier because it’s harder to express the precise purpose in the function‘s name. And the turnover argument applies to any code convention, which makes it a management issue.
  - BurningFrog 7y ago
    Sounds like "team" to you means "dysfunction". I'm sorry you've been in these teams. There are good teams out there!
    - barrkel 7y ago
      Over a multi-year or multi-decade time horizon, it just deteriorates. The oldest codebases I've worked on were 30 years old; the one at my current company is about 10 years old. People come and go, refactorings are started and stopped, bugs are fixed on a tight schedule, and the structure of code divided into nominal blocks slowly dissolves.
hinkley 7y ago
Spreading state mutation out across the system is almost always a bad idea. I would categorize that person as doing DRY badly. Coalescing state transitions should trump decomposition. But it’s often the case that you can do both at once.
- Marazan 7y ago
  That second paragraph should be on the front page of the Redux website.
  - acemarke 7y ago
    Noted! https://github.com/reduxjs/redux/issues/3592#issuecomment-586620584 https://github.com/reduxjs/redux/issues/3592#issuecomment-58... :)
- FeepingCreature 7y ago
  This is wise. Always try to keep your state change points concentrated and legible.
- stormdennis 7y ago
  Can anyone ELI5 the first sentence in each of the two above paragraphs please?
  - hinkley 7y ago
    I can ELI20 easily enough. 5 is a bit tougher. Don’t make me look under every rock to figure out why you changed the stuff I gave you. We aren’t built to deal with chaos. When you put stuff in one end and something random comes out the other end, you have no idea at all what’s going on (until you do, and then they try to put you in charge). You want heaps of code that looks but doesn’t touch. Push the changes to the edges where they are easy to see. If I can trust that things in the middle aren’t mucking around with things all the time then lots of little methods don’t hurt my ability to think about what is happening. (This is not quite the point of Hexagonal Architecture, but they have some common ground. See also early Angular’s philosophy of cooking all input data immediately and passing it around cooked.)
pvorb 7y ago
Ideally the function's name reveals what it's doing. Only if you are tracing down a problem you are required to look into the details. This actually helps to speed up navigating code, because parts of it get meaningful names.
- networkimprov 7y ago
  "Only if you are tracing down a problem you are required to look into the details." This is the common justification, and it's misguided. It turns out that you need to be aware of the "details" every time you look at the code. If, by looking at the code enough, you memorize the "details," it's tempting to move them to a new function with a clever name that tickles your memory. That won't help anyone else. Use comments to introduce blocks of code that need explaining. Use functions in coherent APIs.
  - dragonwriter 7y ago
    > This is the common justification, and it's misguided. It turns out that you need to be aware of the "details" every time you look at the code. No, I don't. I need to be aware of the relevant details, but code in functions with meaningful names mean (1) I can skim a high level overview to see where the details relative to my current interest we likely to be faster and, (2).I can zoom in without distraction to those more easily. > Use comments to introduce blocks of code that need explaining. Comments make a wall of code that is already hard to get an overview of because of its size less legible, decomposition did the opposite.
    - nicoburns 7y ago
      Disagree. Comments can add "section headers", while keeping the code flow linear, so I can skim it just like I skim an article. With methods, I have to jump backwards and forwards and have to remember my place as well as think about the code. This introduces unnecessary overhead.
      - christophilus 7y ago
dragonwriter 7y ago
> Documenting code via single-caller functions is usually a mistake, because to everyone else who looks, the set of functions is an API. That's less the case if the functions are local (either not exported from the module they are included in or actually function-local, in languages supporting nested functions, to their caller. > Both internal and external APIs must be kept coherent. If it's not external, it's not an application programming interface. And, even ignoring that, there is no definition of coherent for which that approximates truth that is inconsistent with single use functions for code clarity and organization. Even languages that don't have mechanisms to make methods/functions truly local/private tend to have conventions for indicating functions/methods not part of the intended-as-public API of a class or module. > Also when readers are trying to understand exactly how some function changes the system state, having to refer to numerous other functions it calls is tedious. Conversely, I find the code having functions with descriptive names helps me not have to read through a bunch of irrelevant code when I'm doing that, which helps me get to the part I need to understand whatever I'm trying to understand faster and with less distraction. Yes, if it's done badly it's problematic, but that's true of literally everything. EDIT: > Also when readers are trying to understand exactly how some function changes the system state Yes, if you are doing relatively unconstrained imperative code that's willy-nilly modifying state, breaking that up into functions passing mutable state around is likely to make it even more incomprehensible. Decomposing into units that are externally-pure functions (that is, while they may do local mutations, they don't modify any state received from the caller), though, is not problematic. In general, except for subsystems dedicated to managing mutable state (and where this function is kept as constrained as possible), I prefer coding in externally-pure functions in general. When you start with that and use it as a constraint on decomposition, decomposition can no longer serve to obscure state modifications. Indeed, it clarifies and more narrowly isolates them.
- networkimprov 7y ago
  > If it's not external, it's not an application programming interface Splitting hairs, I think. Even if not intended, someone else may assume your "documentation" function was meant to be generally accessible (i.e. an API), and call it that way.
nnq 7y ago
I found some simple rules for single caller functions: 1. Use them if they take <5 parametets - no point making a function if you need to pass all the local context into it 2. Use them iff they return something that can be clearly named and have an explainable and understandable TYPE, even in dynamic langs where you don't explictly type things - if you'd end up returning a tuple of 3 arbitrary things that can't be "named as one single concept" it's a code smell 3. Kepp those functions local, maybe define them inside the calling function, at least private if they're methods, anything - if your single caller function becomes unwillingly part of an api, which can esily happen in Python with a _funk() method that users could ignore it's private, you've lost
benibela 7y ago
Pascal has the solution for that. Nested functions: procedure foobar; procedure helper1; begin end; procedure helper2; procedure subhelper2; begin end; begin end; procedure helper3; begin end; begin end; Only the foobar function can access the helper functions.
- dragonwriter 7y ago
  Pascal is hardly unique in that; many languages have nested functions, and every language that has first class functions and local variables/constants has the equivalent of nested functions (potentially with even narrower scope than a whole function) even if it doesn't have a specialty nested function declaration syntax.
- networkimprov 7y ago
  Since nested functions don't create new API, amen.

OJFord 7y ago

> Documenting code via single-caller functions is usually a mistake, because to everyone else who looks, the set of functions is an API. That's a really good way of putting it. I try to avoid it by not writing new functions, but gladly using existing ones, in my implementation of whatever single new one. For example if I'm writing a find_and_update_foobar function, I'll use find_foobar if it exists, and with the right signature, but I won't write it just to implement the one I actually care about; ditto update_foobar. But, professionally I've mainly only used python; so I find it still deteriorates into a mess. (I type hint extensively, but still it only takes some missing hints, or something too loosely - or wrongly - typed.) I haven't used rust professionally/enough/on something large enough to be sure, but my feeling is that it just having a type checker prevents so much mis-refactoring.

nicoburns 7y ago
Interestingly, Rust can be quite opinionated abour code organisation (it makes you keep things in bigger chunks), because some of the compiler analysis doesn't work through function boundaries.
- heavenlyblue 7y ago
  I think this one is bullshit, can anyone else comment on this one? Are there things you may only do iff you using a single function body?
  - nicoburns 7y ago
    A key one is partial borrows of structs (i.e. borrowing only 1 field, and then later borrowing a different one).
zelphirkalt 7y ago
But then what happens to "Your function should be doing one thing and one thing only." (The thing indicated by its name, considering the current abstraction level.)? Perhaps it is not always a bad idea to decompose that function into 2 properly named "steps", which reside in their own functions.

cma 7y ago

> Also when readers are trying to understand exactly how some function changes the system state, having to refer to numerous other functions it calls is tedious. This also applies for small inlined common helper functions. If it folds two or three operations into one, but doesn't have an obvious universally recognizable name, it would often be way clearer to a reader of the code to just put the few operations in directly.

papito 7y ago
Because we write code for other PEOPLE, not the compiler.
- cma 7y ago
  If it is inlined it is usually the same thing to the compiler either way in my example.

marcosdumay 7y ago

> Also when readers are trying to understand exactly how some function changes the system state Yep. That's why changing system state is better kept at a minimum, and at the highest layer possible. Most languages have a very clear public/private marker for determining what is an API and what is just there for convenience (it doesn't have to be literally public/private, for some it's scope, others just tag the names, etc). You don't need to change a code's style just because of it. On the other hand, if the functions do not have logical and atomic meanings, they will do make your code as hard to understand as it will be to name them.

notJim 7y ago

This is such a good, succinct way of putting it. One of the worst code-bases I've ever had to work with was largely so because of so many single-caller functions and far too granular abstraction.

threatofrain 7y ago
It seems that's how a few major libraries are organized, sometimes one function per file.

emsy 7y ago

Since this comment finds a lot of agreement I'm going to play the devils advocate. Private function for the sake of documentation are fine. At least if they are all on the same level of abstraction. If you've ever worked in a team where everyone works this way you stop thinking in APIs, which can be a good thing for internal code.

BurningFrog 7y ago

> Documenting code via single-caller functions is usually a mistake, because to everyone else who looks, the set of functions is an API. Not when you work on a team that does this as a practice. Many teams don't manage to have a common code style, which is a big problem in itself. > Also when readers are trying to understand exactly how some function changes the system state, having to refer to numerous other functions it calls is tedious. The point of breaking out the small functions is to name them so understanding the code becomes easier. This takes some skill and thought, of course, and if done thoughtlessly, it will not be good. But that's true of every practice...

barrkel 7y ago
It especially doesn't work in a team, because under maintenance + employee turnover, these little single-purpose functions start growing hairs, and they end up doing more than they're advertised to by name. Next thing you know, you're fetching the same thing from the database in three different functions, or recalculating the same value repeatedly. Overly granular functions constructed to name blocks of code are a firm anti-pattern in my book.
- emsy 7y ago
  Functions overbearing their purpose can be done in large functions too, arguably even easier because it’s harder to express the precise purpose in the function‘s name. And the turnover argument applies to any code convention, which makes it a management issue.
- BurningFrog 7y ago
  Sounds like "team" to you means "dysfunction". I'm sorry you've been in these teams. There are good teams out there!
  - barrkel 7y ago
    Over a multi-year or multi-decade time horizon, it just deteriorates. The oldest codebases I've worked on were 30 years old; the one at my current company is about 10 years old. People come and go, refactorings are started and stopped, bugs are fixed on a tight schedule, and the structure of code divided into nominal blocks slowly dissolves.

hinkley 7y ago

Spreading state mutation out across the system is almost always a bad idea. I would categorize that person as doing DRY badly. Coalescing state transitions should trump decomposition. But it’s often the case that you can do both at once.

Marazan 7y ago
That second paragraph should be on the front page of the Redux website.
- acemarke 7y ago
  Noted! https://github.com/reduxjs/redux/issues/3592#issuecomment-586620584 https://github.com/reduxjs/redux/issues/3592#issuecomment-58... :)
FeepingCreature 7y ago
This is wise. Always try to keep your state change points concentrated and legible.
stormdennis 7y ago
Can anyone ELI5 the first sentence in each of the two above paragraphs please?
- hinkley 7y ago
  I can ELI20 easily enough. 5 is a bit tougher. Don’t make me look under every rock to figure out why you changed the stuff I gave you. We aren’t built to deal with chaos. When you put stuff in one end and something random comes out the other end, you have no idea at all what’s going on (until you do, and then they try to put you in charge). You want heaps of code that looks but doesn’t touch. Push the changes to the edges where they are easy to see. If I can trust that things in the middle aren’t mucking around with things all the time then lots of little methods don’t hurt my ability to think about what is happening. (This is not quite the point of Hexagonal Architecture, but they have some common ground. See also early Angular’s philosophy of cooking all input data immediately and passing it around cooked.)

pvorb 7y ago

Ideally the function's name reveals what it's doing. Only if you are tracing down a problem you are required to look into the details. This actually helps to speed up navigating code, because parts of it get meaningful names.

networkimprov 7y ago
"Only if you are tracing down a problem you are required to look into the details." This is the common justification, and it's misguided. It turns out that you need to be aware of the "details" every time you look at the code. If, by looking at the code enough, you memorize the "details," it's tempting to move them to a new function with a clever name that tickles your memory. That won't help anyone else. Use comments to introduce blocks of code that need explaining. Use functions in coherent APIs.
- dragonwriter 7y ago
  > This is the common justification, and it's misguided. It turns out that you need to be aware of the "details" every time you look at the code. No, I don't. I need to be aware of the relevant details, but code in functions with meaningful names mean (1) I can skim a high level overview to see where the details relative to my current interest we likely to be faster and, (2).I can zoom in without distraction to those more easily. > Use comments to introduce blocks of code that need explaining. Comments make a wall of code that is already hard to get an overview of because of its size less legible, decomposition did the opposite.
  - nicoburns 7y ago
    Disagree. Comments can add "section headers", while keeping the code flow linear, so I can skim it just like I skim an article. With methods, I have to jump backwards and forwards and have to remember my place as well as think about the code. This introduces unnecessary overhead.
    - christophilus 7y ago

dragonwriter 7y ago

> Documenting code via single-caller functions is usually a mistake, because to everyone else who looks, the set of functions is an API. That's less the case if the functions are local (either not exported from the module they are included in or actually function-local, in languages supporting nested functions, to their caller. > Both internal and external APIs must be kept coherent. If it's not external, it's not an application programming interface. And, even ignoring that, there is no definition of coherent for which that approximates truth that is inconsistent with single use functions for code clarity and organization. Even languages that don't have mechanisms to make methods/functions truly local/private tend to have conventions for indicating functions/methods not part of the intended-as-public API of a class or module. > Also when readers are trying to understand exactly how some function changes the system state, having to refer to numerous other functions it calls is tedious. Conversely, I find the code having functions with descriptive names helps me not have to read through a bunch of irrelevant code when I'm doing that, which helps me get to the part I need to understand whatever I'm trying to understand faster and with less distraction. Yes, if it's done badly it's problematic, but that's true of literally everything. EDIT: > Also when readers are trying to understand exactly how some function changes the system state Yes, if you are doing relatively unconstrained imperative code that's willy-nilly modifying state, breaking that up into functions passing mutable state around is likely to make it even more incomprehensible. Decomposing into units that are externally-pure functions (that is, while they may do local mutations, they don't modify any state received from the caller), though, is not problematic. In general, except for subsystems dedicated to managing mutable state (and where this function is kept as constrained as possible), I prefer coding in externally-pure functions in general. When you start with that and use it as a constraint on decomposition, decomposition can no longer serve to obscure state modifications. Indeed, it clarifies and more narrowly isolates them.

networkimprov 7y ago
> If it's not external, it's not an application programming interface Splitting hairs, I think. Even if not intended, someone else may assume your "documentation" function was meant to be generally accessible (i.e. an API), and call it that way.

nnq 7y ago

I found some simple rules for single caller functions: 1. Use them if they take <5 parametets - no point making a function if you need to pass all the local context into it 2. Use them iff they return something that can be clearly named and have an explainable and understandable TYPE, even in dynamic langs where you don't explictly type things - if you'd end up returning a tuple of 3 arbitrary things that can't be "named as one single concept" it's a code smell 3. Kepp those functions local, maybe define them inside the calling function, at least private if they're methods, anything - if your single caller function becomes unwillingly part of an api, which can esily happen in Python with a _funk() method that users could ignore it's private, you've lost

benibela 7y ago

Pascal has the solution for that. Nested functions: procedure foobar; procedure helper1; begin end; procedure helper2; procedure subhelper2; begin end; begin end; procedure helper3; begin end; begin end; Only the foobar function can access the helper functions.

dragonwriter 7y ago
Pascal is hardly unique in that; many languages have nested functions, and every language that has first class functions and local variables/constants has the equivalent of nested functions (potentially with even narrower scope than a whole function) even if it doesn't have a specialty nested function declaration syntax.
networkimprov 7y ago
Since nested functions don't create new API, amen.