If you're literally using it just once, why not stick it in a local variable instead? You're still getting the advantage of naming the concept that it represents, without eroding code locality.
However, the example is a slightly tricky basis to form an opinion on best practice: you're proposing that the clearly named example function name is_enabled is better than an expression based on symbols with gibberish names. Had those names (x, foo, bar, baz, etc) instead been well chosen meaningful names, then perhaps the inline expression would have been just as clear, especially if the body of the if makes it obvious what's being checked here.
It all sounds great to introduce well named functions in isolated examples, but examples like that are intrinsically so small that the costs of extra indirection are irrelevant. Furthermore, in these hypothetical examples, we're kind of assuming that there _is_ a clearly correct and unique definition for is_enabled, but in reality, many ifs like this have more nuance. The if may well not represent if-enabled, it might be more something like was-enabled-last-app-startup-assuming-authorization-already-checked-unless-io-error. And the danger of leaving out implicit context like that is precisely that it sounds simple, is_enabled, but that simplicity hides corner cases and unchecked assumptions that may be invalidated by later code evolution - especially if the person changing the code is _not_ changing is_enabled and therefore at risk of assuming it really means whether something is enabled regardless of context.
A poor abstraction is worse than no abstraction. We need abstractions, but there's a risk of doing so recklessly. It's possible to abstract too little, especially if that's a sign of just not thinking enough about semantics, but also to abstract too much, especially if that's a sign of thinking superficially, e.g. to reduce syntactic duplication regardless of meaning.
A local variable is often worse: Now I suffer both the noise of the unabstracted thing, and an extra assignment. While part of the goal is to give a reasonable logical name to the complex business logic, the other value is to hide the business logic for readers who truly don't care (which is most of them).
The names could be better and more expressive, sure, but they could also be function calls themselves or long and difficult to read names, as an example:
That's somewhat realistic for cases where the abstraction is covering for business logic. Now if you're lucky you can abstract that away entirely to something like an injected feature or binary flag (but then you're actually doing what I'm suggesting, just with extra ceremony), but sometimes you can't for various reasons, and the same concept applies.
In fact I'd actually strongly disagree with you and say that doing what I'm suggesting is even more important if the example is larger and more complicated. That's not an excuse to not have tests or not maintain your code well, but if your argument is functionally "we cannot write abstractions because I can't trust that functions do what they say they do", that's not a problem with abstractions, that's a problem with the codebase.
I'm arguing that keeping the complexity of any given stanza of code low is important to long-term maintainability, and I think this is true because it invites a bunch of really good questions and naturally pushes back on some increases in complexity: if `is_enabled(x)` is the current state of things, there's a natural question asked, and inherent pushback to changing that to `is_enabled(x, y)`. That's good. Whereas its much easier for natural development of the god-function to result in 17 local variables with complex interrelations that are difficult to parse out and track.
My experience says that identifying, removing, and naming assumptions is vastly easier when any given function is small and tightly scoped and the abstractions you use to do so also naturally discourage other folks who develop on the same codebase from adding unnecessary complexity.
And I'll reiterate: my goal, at least, when dealing with abstraction isn't to focus on duplication, but on clarity. It's worthwhile to introduce an abstraction even for code used once if it improves clarity. It may not be worthwhile to introduce an abstraction for something used many times if those things aren't inherently related. That creates unnecessary coupling that you either undo or hack around later.
I'm speaking solely from a developer experience perspective.
We're talking about cases where the expression is only used once, so the assignment is free/can be trivially inlined, and the attribute lookups are also only used once so there is nothing saved by creating a temporary for them.
Wouldn't you jump to is_enabled to see what it does?
That's what I always do in new code, and probably why I dislike functions that are only used once or twice. The overhead of the jump is not worth it. is_enabled could be a comment above the block (up to a point, notif it's too long)
> Wouldn't you jump to is_enabled to see what it does?
That depends on a lot of things. But the answer is (usually) no. I might do it if I think the error is specifically in that section of code. But especially if you want to provide any kind of documentation or history on why that code is the way it is, it's easier to abstract that away into the function.
Furthermore, most of the time code is being read isn't the first time, and I emphatically don't want to reread some visual noise every time I am looking at a larger piece of code.
> Wouldn't you jump to is_enabled to see what it does?
It determines whether the thing is enabled. Or else some other dev has some 'splainin' to do. I already understand "what it does"; I am not interested in seeing the code until I have a reason to suspect a problem in that code.
If the corresponding logic were inline, I would have to think about it (or maybe read a comment) in order to understand its purpose. The function name tells me the purpose directly, and hides the implementation that doesn't help me understand the bigger picture of the calling function.
Inline code does the opposite.
When the calculation is neatly representable as a single, short, self-evident expression, then yes, I just use a local assignment instead. If I find myself wanting to comment it - if I need to say something about the implementation that the implementation doesn't say directly - using a separate function is beneficial, because a comment in that function then clearly refers to that calculation specifically, and I can consider that separately from the overall process.
Ah, but what exactly does "enabled" mean in this context? Might seem nitpicky, but I might very well have a different opinion than the person who wrote the code. I mean, if it was just `if foo.enabled ..` no one would put it in a new function.. right? :)
I would say a comment does the same, and better because it can be multi line, and you can read it without having to click or move to the function call to see the docs.
And you can jump past the implementation, iff it's short and "tidy" and enough.
Yes, at some point it should be moved out anyway. I'm just weary from reading code with dozens of small functions, having to jump back and forth again and again and again
> If the code is working, it means what it needs to mean.
No. Working code says nothing about the meaning of a label, which is purely to inform humans. The computer throws it away, the code will work no matter what you name it, even if the name is entirely wrong.
> Why do you jump to look at the other parts of the code? Did it fail a test?
Because people pick bad names for methods, and I've been hurt before. I'm not reading the code just to fix a problem, I'm reading the code to understand what it does (what it ACTUALLY does, not what the programmer who wrote it THOUGHT it does), so I can fix the problem properly.
I have absolutely done this myself in the past and confused myself with bad names. Any criticism I apply to other people also applies to myself: I am not a special case.
Naming things is hard! Even if you're really good at naming things, adding more names and labels and file separation to a system adds to the complexity of the system. A long function may be complex, but it doesn't leak the complexity into the rest of the system. Creating a function and splitting it out is not a zero cost action.
I write long functions when long functions make sense. I write plenty of short functions too, when that makes sense. I'm not religiously attached to function or file size, I'm attached to preserving the overall system structure and avoiding stuff that makes easy bugs.
So my claim is that you do this less often than you claim to. There is some cutoff where you trust the code enough to not investigate it further. I'm of the opinion that this trust should generally be pretty close to the actual thing you're working on or investigating, and if it isn't that's a cultural issue that won't be solved by just "prefer to inline".
