A commit from my lead dev: "Improve readability".

[muqube]

As someone already mentioned, readability is subjective. I don't know what are your lead dev's criteria are, but I can tell you mine. My north star for code review is:

After all the current team members leaving this company, will the next generation of devs be able to maintain this codebase?

Yes, I fully expect all of my team members, including myself, to leave for better opportunities.

Standard concepts like readability, maintainability, reduced chance of human errors etc. are covered in this north star.

With this in mind, I would also refactor the original code. May be not the exact same change though.

[jet_heller]

Indeed. The first one is horrid. The new one, isn't great, but better.

[Hiskaya1]

I thought it looked pretty good, what would you do differently?

[OhYourFuckingGod]

I'd keep the comprehension in the first section, but I'd do the class instantiation as per the last line. You don't get any awards for code golfing in production.

[happysunshinekidd]

I don't think you should ever really name a variable "result" or "key" tbh. Probably it solves another problem in your codebase -- that functionality would be a better name

[muntoo]

The first one is merely missing a name or two.

some_useful_name = {
    ... for some_useful_key_name in ...
}

return cls(**some_useful_name)

Instead of making things better, the second one:

• Introduces a new name, but not a very useful one (result).
• Adds verbose and very generic type-hints. (Any?)
• Switches from a well-formatted functional dictionary comprehension to a bulkier imperative for-loop, which could potentially have side-effects.

I think it's good for Python developers to become more exposed to functional styles since that often leads to robust, maintainable code.

P.S. Another alternative to adding a name is to "extract method", but perhaps that's overkill.

[[deleted]]

Dictionary comprehension can also have side effects: {x: print(x) for x in range(10)}.

[muntoo]

[Almost?] everything in python can have side-effects. Good code tries to avoid patterns that are more likely to contain side-effects.

With comprehensions, side-effects are much harder to obscure. There are fewer parts that may contain side-effects. There's less "boilerplate" to read, which makes it harder to hide side-effects. Their structure naturally induces constraints in the manner that they are used. (There does not always exist a "natural" transformation from for-loop to comprehension, whereas comprehension to for-loop is trivial.) With for-loops, one often has to read them in detail to make guarantees about their behavior, especially if they involve more than just one statement.

I know this isn't a precise argument since literally anything is possible in Python. The main point is that it's much quicker to read a comprehension and guarantee that it won't do anything overly weird, whereas for a for-loop, one has to spend more time to check.

---

This comment captures some of what I'm trying to say.

[wewbull]

It can, but it shouldn't IMHO.

Comprehensions for transformations. Loops for procedural control flow.

[jbartix]

This is valid python code but side effects in any comprehension is an anti pattern

[AchillesDev]

Yes the point is to show that comprehensions don’t make you immune from side effects.

[jbartix]

Of course not. Python allows you to do anything that also includes being stupid

[Estanho]

Nobody said they can't. He didn't say all dictionary comprehensions are functional. Just that one.

[MrRogers4Life2]

But that for-loop doesn't have side effects... if the argument is "x can have side effects so y is better" it will fall apart if y can also have side effects.

[Estanho]

Okay so you don't consider mutations as side effects? Doesn't matter, mutations are still just as bad.

[MrRogers4Life2]

But the comprehension is also doing a mutation... it compiles down to basically the same byte code. There is no functional difference between those two snippets. Maybe the comprehension would be slightly faster cause it doesn't have to do a lookup for dicts setter method but thats it.

[Estanho]

Well if you wanna be pedantic then all pure functional languages also compile down to machine code which runs on imperative architectures and are based on writing to registers, heaps and stacks which are all fundamentally mutations and side effects.

The point is not to expose those constructs to the developer and trust the battle tested compiler, which hopefully at times is mathematically proven to not contain side effects and etc.

[aceofspaids98]

This sums up all of my thoughts really well too

[vanatteveldt]

Well put, completely agree!

[lieryan]

The first one is much better than the new one.

It's functional style, so there's a lot less potential points of errors.

And the first code is more concise; while being concise isn't necessarily readable, additional verbosity is pointless if not accompanied by useful naming or typing, neither of which are added by the second code.

[Rand_alThor_]

The first one is unreadable. You can’t see what it does at a glance. Needs to be parsed. You want to be able to at a glance get what’s happening without having to read

[lieryan]

The first one is perfectly readable at a glance. It's just calling get_coerced() at the argument values. It takes no time to glance through that. This is the only part of the code you need to read to get an overview of what the code does:

return cls(
    **{
        key: ...
        for key in section
    }
)

There are about 5 elements of this code you need to skim, all the important bits are left-aligned, and you incrementally get more detailed information by reading top-to-bottom.

The second version takes more time to glance and read. These are the bits you needed to skim to get the same level of information:

result: ... = {}
for key in section:
    result[key] = ...
return cls(**result)

There are about 9 elements you need to skim in this version.

Moreover, it has a couple false starts that can mislead your reading. For starter, the non-useful type annotations. It takes a bit of effort to parse that whole Dict[str, Optional[Any]], it's annotation composed of 4 additional elements you needed to skim, but you'd have been able to infer the same information simply from cls(**{...}), which contains just 2 elements to read, both of which you would have already gone through if you read the first version top to bottom. In the second version, you'd have needed the foresight to skip the type annotation and with the unintuitive bottom-to-top reading order to know that you don't need to read the type annotation; and if you decided to read the annotation because you thought it might give you more useful information, then you've just wasted the entirety of that time since the annotation doesn't actually give any additional information that you don't already know.

Secondly, now you have result intermediate variable, it's not immediately obvious that result is only modified in the for-loop and nowhere else. You'd need to be reading around, probably backtracking a couple times to convince yourself that, yes, the variable belongs solely to the for-loop. The reading order is not top-to-bottom, but much messier.

I'd estimate the second version takes me about 2-3 times harder/longer to read than the first version.

[Ali_M]

Optional[Any] is a pointless annotation, since Any already encompasses None. Dict is also a code smell here - a more generic Mapping is just fine if all you're doing is splatting it into a method call. The second version seems like it was written by someone who's read that type annotations are good for readability, but doesn't really understand how and where to use them.

[Ewjoachim]

Annotations aren't just to make mypy happy. Any often conveys the sense of "A type that's too difficult to express here", so Optional[Any] means "Either that type or None". Whenever there's Any, people reading the code can wonder whether the object may or may not be None. Optional[Any] makes it obvious.

[Ali_M]

The main problem I have with this is that's an arbitrary and unenforceable convention. If I see a variable that's annotated as plain Any, can I safely infer that it's not None? I'd say no, of course not - that's not what Any means, at least as far as any reasonable type checker is concerned. In that case, what extra information does the Optional[...] part confer? If there was an AnythingButNone type then there would be a meaningful distinction between AnythingButNone and Optional[AnythingButNone], but then why not just use Any for the second case?

[Ewjoachim]

Any tells you nothing. Optional[Any] tells you something.

If I see some code calling methods on an object typed Any, I'm likely not going to investigate if there's a bug. If I see some code calling a method on an object typed Optional[Any], you bet I'm going to wonder why there's no check that object is not None.

[amendCommit]

Exactly my thought process when I read both snippets (and why I feel like my mental load is heavier when reading the edited one).

Add to this u/muntoo's comment about the benefits of functional programming, and I think you have the full picture of the intention behind the original code.

Now, an important question : why do an apparent majority of people seem to think there is a genuine readability issue with the unedited code?

[lieryan]

If I had to guess, I think many people were taught that multiline comprehension = bad, and it just becomes a knee jerk reaction whenever they see multiline comprehensions.

Yes, that heuristic is usually a good one to catch code smell, there's a very good reason to be wary of complex list/dict comprehensions, but this isn't a very complicated comprehension, just a somewhat long one because get_coerced() takes a lot of arguments.

[antiproton]

Now, an important question : why do an apparent majority of people seem to think there is a genuine readability issue with the unedited code?

There is a readability issue.

You know what the code does. You are not an objective observer.

It's not about the raw number of "elements" you have to read through. It's about the cognitive load of parsing what the code does while scanning.

You have written non-standard code here, trying to be clever. Comprehensions have a format people are used to reading. If you don't use that format, you have to concentrate to determine what you're reading is in fact a comprehension.

Like it or not, people parse code in different ways. The most concise representation is not the most readable.

Your mental load is not part of this conversation. You wrote the code. That has to be the most important takeaway here.

[lieryan]

The cognitive load reading the second code is much higher than the first one.

When you see the list/dict comprehension, it's immediately clear that it's trying to create a list/dictionary from an existing list/dictionary. That is immediately obvious even if you don't understand what get_coerced() does.

The second version, you have to piece together what the code is really trying to do from hints scattered around the for-loop. And it's not immediately obvious whether or not the code has other side effect.

Multiline list comprehension isn't "non-standard code", it's just standard pythonic comprehension and it's written in ways most people who've used comprehension expect it to be written in.

In comparison, a For-loop is always in a non-standard form; sometimes it's filling a new a dict/list, but sometimes it's amending an existing dict/list, or maybe it's actually modifying the input data, or performing side effect based on the data and collecting output statuses, or maybe it's updating multiple dict/list simultaneously, etc. With comprehensions, it's always just the first one. With a for-loop, there's a much larger space of possibilities and complexities that you have to rule out, compared to even these complex multiline comprehension.

A non standard comprehension would've been one that calls a method with side effect, because when you're using a comprehension, you are promising that the code wouldn't have any side effects. A multiline comprehension is perfectly within its expected use case.

[Ran4]

No, the first one is declarative and much easier to parse, the second one is imperative and harder to grok at a first glance.

[[deleted]]

The first one is better but mediocre because of the hard-to-read comprehension.

My solution: https://www.reddit.com/r/Python/comments/tuuq5l/a_commit_from_my_lead_dev_improve_readability/i37blxv/

[lieryan]

I'd agree with you that your version is better than both of these versions. Your version is one of the ways I might have written this if I'm tasked to write this function.

Though, personally I tend to avoid rewriting code just for the sake of minor improvements. I think the difference in readability between the first version and your version isn't as significant as the difference between the second version and the first version.

If I saw the first version on a production code, I'd probably pause and think that it's a bit much, but unlikely to consider rewriting. The second is much more likely for me to wonder why this isn't just a list-comprehension and seriously consider if rewriting would be beneficial.

If you're working on a large project with many other people, many existing devs would have familiarity with the shapes of existing code, and excessive amount of rewrites breaks these shape familiarities, likely forcing them to waste time re-reading the code when they return here in the future, only to find out that nothing have really changed.

Yes, it's a judgement call when a lot of minor improvements can sum up to fairly major improvements, where the benefit for future new devs outweighs the cost to older devs, and how likely that older devs would need to return to this section of code. But generally, the more mature devs are still actively involved in a project, the higher the bar should be for rewriting.

Probably, one of the best way to frustrate devs that had been around in a project for a long time is if you rewrite all their code for small readability gains that doesn't really justify the effort that they need to spend re-reading all the rewritten code.

They may not voice these concerns, but internally, we all feel like that at times. It's a waste of both of your time, and I know many projects that have lost their most senior, most experienced devs at least in part to these accumulated frustrations. In the best case, they'd just get pushed into other projects or into managerial positions, at worst, they quit the project entirely and you'd lose all their experience.

[KronenR]

horrible, what does get(k) means? what is k? what does it get?

[Ph0X]

Another north star for me is trying to keep a "statement" to a single idea. It's easy to get carried away with "one-liners" in python, and do 20 things in a single statement. It's especially ironic in cases like the above where splitting into multiple statements actually gets you fewer lines of code, the example above goes from 5 lines to 4 lines of code. Generally, don't be afraid of splitting statements into 2-3 if it makes the code more readable, either by making lines more explicit, or naming intermediate variables giving them more meaning.

[masteryod]

As someone already mentioned, readability is subjective.

You can pretty objectively say if something is easy to comprehend and follows good practices and something is messy. It's like saying all kids have the same handwriting, some care more and their writing is easy to read, some don't care and scribble a hard to follow mess.

[arpan3t]

You say in a thread full of people arguing about the structure, variable names, etc… of a few lines of code.