[emn13]

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.

[lazyasciiart]

Pretty sure every compiler can manage optimizing out that method call, so do whichever makes you and your code reviewer happy.

[joshuamorton]

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:

    if (
        x.is_enabled ||
        x.new_is_enabled ||
        (x.in_us_timezone && is_daytime()) ||
        x.experimental_feature_mode_for_testing 
        )...

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.

[sgarland]

> Now I suffer both the noise of the unabstracted thing, and an extra assignment.

Depends on your goals / constraints. From a performance standpoint, the attribute lookups can often dwarf the overhead of an extra assignment.

[joshuamorton]

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.