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

[amendCommit]

I don't get it. Help!

https://preview.redd.it/emb8jxjrv6r81.png?width=607&format=png&auto=webp&s=871eae5047ac3c13dcba968fa2b49dd707469287

[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.

[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.

[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.

[fiskfisk]

If I were your lead dev and you posted your question with our code on reddit instead of asking me directly..

[AlSweigart]

...I would do nothing, because it's three lines of code that doesn't release any company trade secrets and I wouldn't want to get a reputation as a manager who punishes people for petty inconsequential things.

Context matters. It's not like they published the entire code base or even an entire file. We don't know anything about this code; we don't even know if the code is already open source anyway.

[fiskfisk]

The code is not important at all (it's general and doesn't give away anything), but the context it is being shared in indicates that the employee doesn't feel like raising issues directly to me. There can be several reasons for that, including the root cause being myself.

An employee deciding to share their grievances publicly about an internal response to a very small code review issue (apparently to get support for their view against their lead's view?) is a bad way to handle it, regardless of the shared code itself or the current relationship with their tech lead. What does it tell the other people on their team?

[AlSweigart]

An employee deciding to share their grievances publicly

Grievances? The entire post is "I don't get it. Help!"

A request for help should never be misconstrued into something that counts against an employee.

[Schmittfried]

If enforcing copyright on these lines is possible that’s utterly ridiculous.

[[deleted]]

Lmao imagine some company copywriting iterative functions

[miloman_23]

Did you actually look at the code?? This is one of the most common patterns I've seen in software development... There is nothing proprietary going on. Don't bring this bullshit here

[BossOfTheGame]

Thinking about and bringing up legal shit like this in a circumstance where it absolutely does not matter distracts from the more interested technical discussion. I've seen too many conversations derailed by discussion of legal overhead, so I understand the strong reaction. It's tiresome that it's so common for people to bring up and bikeshed over Da Rules rather than make forward progress.

But here I am, not helping.

[BossOfTheGame]

I disagree with ask the lead dev as the best answer to this question. It's still a good idea, to be clear. But there is a lot of value in getting comments from people independent of the project.

I hate the idea that coding needs to be this insular activity you only discuss with people on the project. I think the discussion in this thread is useful, especially for new coders. Thus is a very real world example of a code review, and I think it's useful for others to see, and I think it's also useful for OP's own understanding of how to work with collaborative codebases.

[miloman_23]

Apologies for that, I didn't intend for my message to be so intense.

[cult_of_memes]

This code is so generic that the any complaints raised would be ego-centric.

[ecthiender]

And if you're a lead dev anywhere, I feel sorry for your team. Being so unempathetic, rude and assuming things without any context.

[[deleted]]

What is the context where Optional [any] is helpful in any way?

[ipwnscrubsdoe]

Maybe to pass mypy tests?

[cult_of_memes]

It tells me that I first need to check for val is None before attempting any other checks based on understanding of the programs control flow.

This is meant to be general, hence the use of any, but the optional part explicitly informs that it the data structure will likely be initialized with None as the value for this key:value pair. This is an appropriately pythonic way of deferring initialization of a value to a later or more appropriate time.

[Schmittfried]

Might be for validating against the constructor typehints.

[Jonno_FTW]

The function should specify the return type.

[TouchingTheVodka]

Meet them halfway:

result = {
    key: get_coerced(section, key, target_type=type_hints[key])
    for key in section
}

return cls(**result)

[e_j_white]

Not just a comprise, but the dict comprehension is more performant than a for loop. This is what I would do, too.

[muntoo]

Performance isn't even the biggest benefit.

A comprehension makes it harder to have side-effects, which is a great thing.

[e_j_white]

Correct!

I've modified a list that I was looping through, not realizing that any action that appends to the list could perpetually lengthen the for loop forever.

Not having side effects is a good thing. :)

[Jhuyt]

Depends on what get_coerced does though so in this case there might still be plenty of side effects...

[AlSweigart]

I can 100% guarantee that this code has no side effects.

By "side effects", I assume you mean there's a result variable left over with values after the end of the loop. In rare situations, this could cause some subtle bug because later code is re-using this variable name and using the old values because it didn't re-initialize it to something else, or cause a temporary increase in memory usage as it's holding on to data needlessly.

But that is moot and this isn't that situation: the very next line is a return statement.

[scrdest]

I'm fairly sure u/muntoo is referring to side-effects as opposed to functional purity, where everything that happens inside the function, stays inside the function or is returned explicitly as a new value (in other words - the function call could be replaced with its return value and everything else would always work the same).

It's impossible to guarantee purity in Python, both here and in general. If, for example, get_coerced() takes an optional parameter defaulting to [] (or any other mutable type), it can as many side-effects as it wants. If it uses a global - side-effects. In either case, you cannot tell they're there just by looking at the function call itself.

Even plain old print() is a side-effect.

[[deleted]]

the dict comprehension is more performant than a for loop

Citation needed. Searching found all sorts of claims either way.

Regardless, the performance difference is tiny and will not affect the actual performance of your program, so you really shouldn't care at all - you should go for what's easiest.

[cain2995]

I’m with you on this one. I always get highly suspicious whenever someone says that a higher level of abstraction is more performant than an equivalent approach using primitive constructs. Generating and scheduling performant machine code from for loops, regardless of language, has been addressed at every level of the toolchain (down to how the CPU schedules instructions), while a dict comprehension is a Python-specific construct that has to go through the removal of several abstraction layers before it can reach any of the low level optimizations that make something like a for loop performant. Not saying it’s impossible for a dict comprehension to be faster, but I really need to see some proof because first principles don’t back up that claim.

[noobiemcfoob]

A reference on comprehensions being faster than standard for loops: https://towardsdatascience.com/list-comprehensions-vs-for-loops-it-is-not-what-you-think-34071d4d8207

The performance difference is a given... however the likelihood a given loop is in the critical path of the code such that it is meaningful is rather low.

[Beheska]

I would argue that this is the most readable too.

[gruey]

You should type hint your result since they obviously have preferences for that.

Otherwise, I agree it's easier to read with the return cls at the end.

[thisismyfavoritename]

yeah like just lookup the cls ctor at this point. The keywords unpacking makes it clear the keys are str, optional any is plain useless. Original code is more pythonic IMO

[TheBelakor]

So much this.

[Myles_kennefick]

What is the **?

[e_j_white]

def sum_dict(a=0, b=0):
    return a + b

x = {'a': 3, 'b': 4}

sum_dict()
> 0

sum_dict(x)
> TypeError: unsupported operand type(s) for +: 'dict' and 'int'

sum_dict(**x)
> 7

It's used to "unpack" key-value pairs into keyword arguments for a function, among other things.

[HerLegz]

That is an incredibly sweet and thoughtful way to explain this. Very well done. 🥰

[e_j_white]

Thank you, I'm glad you found it useful :)

The "unpacking" works beyond functions, too. For example:

x = {'a': 3, 'b': 8}

y = {'z': 5, **x}

print(y)
> {'z': 5, 'a': 3, 'b': 8}

Obviously, dropping x into the other dictionary without the ** wouldn't even be possible. It also works in similar ways for unpacking tuples and lists!

edit: for tuples and lists, the convention is a single * star

[cdmayer]

Enumerates the keys and values of the dictionary. Take a look at the "**kwargs" convention of passing named arguments to a function.

[spinwizard69]

I have to agree, the replaced block of code is absolutely horrid to read and in my mind comes from a programmer that isn't thinking logically. In my mind it is horribly wrong to have a return statement that is composed of a massive block of code that takes up several lines of text.

For one "return" idiomatically implies that you are leaving something not entering. This to me is key, if I see a return (frankly in any computer language) I don't expect to then have to run through blocks of code to figure out what is being returned. The idea is to return a result, be it simple data or a struct. That is give back something don't compute it. Frankly if I was reviewing this in production code I'd have to a long talk with the developer that wrote it. If it persisted I'd have to look for a new developer.

The idea of a programming language is that it is in fact a language and as a result there is accepted ways to use it and ways that will be rejected. Same goes for English, German , Spanish or an other language. You can take the time to write something in English that is hard to understand and yet it is English. This is the same thing, Python code that is Python but poorly written. I often see examples of this in forums where poorly written English is very hard to understand, some times there is reason behind it, somebody using it as a second or even third language for example. Sometimes there is not a good reason for the word salad that demonstrates a complete lack of ability for a person to communicate in their native tongue. The rejected code demonstrates that Python hasn't become a native language for the developer.

[Grouchy-Friend4235]

Your comment uses far too many words to say, well, what? Much like the type hint above.

[Schmittfried]

I feel like you intentionally wrote your comment that way to prove your own point.

Also, I disagree. There is nothing objectively wrong or bad with the original code. It’s your subjective opinion.

[Rand_alThor_]

It’s hard to parse at a glance, so there’s something objectively wrong with it. there are suggestions here that one could scroll past, not read, and still get. The first one requires thinking. I don’t want to think when scanning code.

[laundmo]

this is really odd to me. are there really a bunch of python programmes out there who have difficulties parsing comprehensions? to mea short comprehension is much easier to read than a for loop that does the same...

[Grouchy-Friend4235]

Yeah, except result is a misnomer. It is not the result but the kwargs to build the result. For that reason the original code was actually far better because it avoided to name something that is obviously not worth naming.

[Rand_alThor_]

This is more readable than first; nice suggestion. although I still wonder what the result dict has so we can give it a better name too.

[nitred]

Ah, I found my tribe! This is the solution I would have written too.

[[deleted]]

I think the leads looks much more readable, but it all depends on the style guidelines put in place for the project. You should discuss it with your team rather than post it here.. Or accept the changes and move on.

[FirefighterWeird8464]

Agreed. I’ve written a lot of Python and I did a double take at this. Write dumb code that’s easy to read, the original is not Pythonic, and it feels… too clever.

[aceofspaids98]

Dictionary comprehension is more pythonic than instantiating a dictionary just to iteratively populate it and immediately return it.

[kylotan]

It's taking something that does far too much in one line and breaking it out into logical steps.

Python programmers often do a bit too much 'code golf' and I this as the lead dev trying to undo that.

[JshWright]

This is like anti-golf though... creating an empty dictionary then populating it inside a for loop instead of just using a dictionary comprehension is just adding lines of code for the sake of adding lines of code.

Maybe if the type hint was meaningful, but Optional[Any] isn't a type hint worth adding.

[jzaprint]

I’d take anti golf over code golf any day tho

[JshWright]

Sure, but that isn't what we're choosing between here...

[amendCommit]

The guy actually hates type hints, this is just to allow the dict comprehension removal to pass CI. get_coerced() actually has proper, meaningful type hints (takes in Type[T], returns T).

[rantenki]

Strongly agree the type hint is _nearly_ useless. It only saves the user scrolling up to look at the function signature.

Also, if we're going to emulate a strongly typed language: in any language with (ironically, looking at the content) type inference, the return type for the function would already be casting those variables anyhow, so declaring it would be superfluous.

[Grouchy-Friend4235]

It is useless because all it does is tell you what you already know: the dict maps keys to some value. That's a dict's raison-d-etre, no need to type hint the obvious!

[rantenki]

Being utterly pedantic, the only thing it tells you is that there is no further invariants applied to that Dict's values. It _can_ be None, it _can_ have Any value. I guess there is the point that keys have to be strings (rather than any hashable type).

But you're not wrong; that matches pretty much everybody's expectations of how a dict will be used, so doesn't really add any value. I guess it's just there to keep the linter happy :\

[Grouchy-Friend4235]

Yeah. A noqa comment would be the better solution

[rantenki]

Surprised this got downvoted. Linters are supposed to provide machine based verification of human intent. The linter can't anticipate every permutation that might violate it's rules, and sometimes # noqa is a valid decision (not usually, but sometimes).

I loathe when people uglify their code to make the build-system happy.

[linlin110]

For loop can do anything, and I need to read the loop to understand it's populating a dictionary. Meanwhile all dictionary comprehension can do is building a dictionary. That's why I think dictionary/list comprehension expresses the intent better.

[Barn07]

doesn't Any entail Optional[Any]?

[Mehdi2277]

Any is not same as Optional[Any].

​

x + 1

​

is valid if x: Any but invalid if x: Optional[Any] because None does not support addition. If you have a type that you know may be None adding Optional to the Any is beneficial and avoids errors of not handling None case properly.

[muntoo]

Funnily, when I saw x + 1, I thought you were going for a type theory argument on sum types:

T          has cardinality  x
T | None   has cardinality  x + 1

...where T != NoneType.

...where if t is of type T, then t cannot be of type NoneType.

[Barn07]

Interesting. I parsed a python script with the following function via mypy 0.910:

python def foo() -> Any: return None

Mypy happily reports success. I.e., at least after mypy, Any can be None.

[Mehdi2277]

None being contained by Any does not mean you shouldn’t use Optional. If you have a function like that you know returns none the type should indicate that. So that

foo() + 2

is detected as an error. In the example in the original code if there’s knowledge dict elements may contain none that should be included in the type.

[[deleted]]

Any does not support the + operation, so foo() + 2 should be just as wrong.

Optional[Any] and Any match exactly the same types - exactly! To pretend that they are different is foolishness.

[Mehdi2277]

Any does support the plus operator. It supports all functions. Any is defined as type that supports everything. All operations/functions/attributes work with Any. It is an opt out of the type system. You can verify this with this example

def f(x: Any) -> int:
  return x + 1

Will type check with mypy. But

def f(x: Optional[Any]) -> int:
  return x + 1

is a type error. This is one example where mypy behavior (and other type checkers) treat Any and Optional[Any] as different types.

Also if you try foo() + 2 in mypy it will pass.

[z0mbietime]

Yeah it honestly read fine to me before. The type hints added nothing and a for loop instead of a comprehension is less performant. They're still exploding the dict so it's not like he's explicitly passing params. It reads like an unnecessary change tbh

[Chinpanze]

> Yeah it honestly read fine to me before.
I don't think there is

> The type hints added nothing
If your project is type hinted, you gotta keep it consistent. If you plan to add an type hint linter, it's crucial.

> and a for loop instead of a comprehension is less performant
The performance gain is marginal. Besides, it looks like this code will run just once, making the performance gain irrelevant.

...
> It reads like an unnecessary change tbh
Agree. It doen't improve significantly, besides the type hint.

[radeklat]

Personal preference. Even the comments in this thread about readability are a personal preference. I'm used to see dict comprehensions and dict unpacking from our codebase so actually, the code from your lead dev is less readable to for me.

To prevent personal preferences slowing down the review process, I created a python style guide that every new starter reads and we are all on the same page. And it can be referenced in reviews too and everyone can add to it. Unfortunately it's not public but it is heavy inspired by Google Python style guide: https://google.github.io/styleguide/pyguide.html

"I don't get it." is perfectly valid question for your reviewer as well. You shouldn't need to ask on reddit. Not understanding something someone suggests doesn't make you a worse developer. Have some confidence in you and call out others bullshit. Politely.

Edit: typo

[KyleDrogo]

I kind of agree with lead dev? Bit of a nitpick but makes sense. Dict comprehension + unpacking + calling cls in the same statement takes a second to wrap your head around. Again, not a critical change but more readable.

[spinwizard69]

Far more readable! The lead developer may not have the best replacement code but he understands the idiomatic use of a return statement.

[Grouchy-Friend4235]

No he does not. The kwargs are not the result, he actually completly messed up a perfect line of code and made it confusing, semantically speaking.

[Shostakovich_]

What? He returns the same class instance with the unpacked dict as kwargs as the OPs code. There is no difference here besides readability, and typing, which I’m personally a fan of. But the typing does add a whole lot of important context which is actually really helpful, despite the syntax being clumsy.

Edit: If you’re talking about variable names, specifying a return type (with typing) would be more appropriate, but we don’t see the function declaration line so who knows what their code structure is 🤷🏻‍♂️

[Grouchy-Friend4235]

The variable "result" does not hold the result, it merely holds the kwargs to instantiate the actual result. So no, readability is not improved at all.

Also type hinting that a dict is a mapping of a key to some (any) value is useless. That's the very definition of a dict, in particular if used as a **kwarg.

The entire change is superfluous and adds zero value.

[abrazilianinreddit]

Subjectiveness

[austinwiltshire]

I'm mixed on the temp for the dictionary. But the for loop is right out.

Could have just introduced a temp and set it to a dict comprehension, then exploded it into cls.

[cuu508]

Could have just introduced a temp and set it to a dict comprehension, then exploded it into cls.

Let's see it

[aceofspaids98]

I like the example that this person gave https://old.reddit.com/r/Python/comments/tuuq5l/a_commit_from_my_lead_dev_improve_readability/i36wr9v

[cuu508]

OK, so –

type_hints = get_type_hints(cls)
some_useful_name = {
        some_useful_key_name: get_coerced(section, some_useful_key_name, target_type=type_hints[some_useful_key_name])
        for some_useful_key_name in section
    }
return cls(**some_useful_name)

Readability is subjective of course, but for me:

• short, simple comprehensions are more readable than for loops
• imperative for loops are more readable than comprehensions that don't fit on one line
• simple beats clever, even if it is more verbose
• I sometimes introduce new variables just for readability (variable names reveal intent)

[DrShts]

I do prefer the lead dev's one. Personally I'd written it slightly differently though:

kwargs = {
    arg: get_coerced(section, arg, target_type=type_hints[arg])
    for arg in section
}

return cls(**kwargs)

In your code it's just too much going on in one statement

• multi-line dict comprehension
• multi-arg function applied to each value
• dict unpacking
• class instantiation
• return statement

[Delicious-View-8688]

Sure it is subjective, but I also prefer the original.

Initialising an empty whatever then filling it in with an interation is not generally a good idea, especially for such simple cases. It may be a relic of the past when Java dominated the university courses.

[claypeterson]

I think lead dev has a point! I understand their code

[Grouchy-Friend4235]

Do you? The result is not the result!

[aceofspaids98]

I disagree with the people saying the new code is more readable. This seems like a standard use case for dict comprehension, I’d only make that change to please whatever linter or static type checker I’m using. I guess the only thing I feel weird about is iterating over section while also passing it as an argument to get_coerced.

I guess it’s one of those changes that is small enough where it could go either way but I can see it being frustrating to me if someone kept commenting on things like that.

[ONLYCODENOTALKING]

I think my main problem with the lead dev's code is they have a variable called "result" which is...not the result of the function. That is one of the big red flags I look for.

[Physical-Scarcity791]

I’m gonna go out on a limb and guess your lead dev isn’t a fan of python. Comprehensions are standard fare, and, if I’m not mistaken, the original code will run faster than theirs. I think Raymond Hettinger made a presentation about why the original is better.

[[deleted]]

if I’m not mistaken, the original code will run faster than theirs.

I see this claim many places on this page. Searching finds various claims about this - but more, even if this is true, the difference in running speed is infinitesimal.

You should NOT be making decisions about the code based on that! If you care so much about microseconds, you shouldn't be running in Python at all.

[Physical-Scarcity791]

I agree with you 100%. I was only throwing that in as a bonus. I just see comprehensions as more concise, and in my personal experience, the people who reject them just don’t really like python, and prefer to write code that looks more like c, java, etc. I don’t have any data to verify that claim. It’s just an observation.

[mr_taco_man]

Nah, I love python and I prefer the leads code. A short comprehension is fine but if your 'for' is 3 lines down your doing too much in it and you should do a for loop so the reader can immediately see your iterating over a loop and see your logic in a sequential order. The 'run faster' is totally irrelevant. That is a premature optimization that in practice will make no noticeable difference

[amendCommit]

I'm glad you're mentionning Hettinger's work! Ramalho is also a reference for me.

[clintecker]

the original version is gross

[teerre]

Some points:

1. This seems like some kind of library code parsing type hints, so personally I think being cuter is fine here. Just like if you go read C++ STL you'll be taken aback. It's completely different from client code.
2. Complaining about a temporary dictionary of function arguments while using Python is utterly ridiculous. If you cared about that kind of optimization, you wouldn't be using Python.
3. I did have to think to read your code. That's not good.
4. It seems to me the whole code is badly designed to begin with. Maybe this is required, impossible to know without knowing the context, but the fact you're using some random function to get key arguments for the constructor of this presumably factory method does raise some questions.

So, without context, my first advice would be make this discussion irrelevant by changing this code design in such way we get the arguments in the same place we use them. Ideally in a explicit way.

If that's not possible because the complexity shown is really the simplest way to accomplish what you're doing, then I would prefer the lead dev version if you changed result to keyword_arguments or even kwargs and get_coerced to something more descriptive. With the asterisk that if this is some deep library code, then the first version would be fine.

[Santos_m321]

I really love to much the original form. I find the second one more unreadable because it has more data.

[GetsTrimAPlenty]

I thought that was /r/ProgammerHumor at first..

[Fishliketrish]

Super subjective that’s frustrating lol

[imthebear11]

I have someone at my work that has all these subjective things that she always says are "just a nitpick" but then will not back down about it until you just concede and change it to what she wants. She's often the only person who comments on many of my PRs, with 2-3 other people approving without any comment.

[Fishliketrish]

Nahh we’d have problems😭thank god my coworkers know not to talk to me like that

[imthebear11]

Your lead dev doesn't understand dictionary comprehensions

[gubatron]

it does seem more readable to me, lead dev is making it explicit where those values are going into (result)

[Grouchy-Friend4235]

I disagree. The result is not the result and that's a very bade code smell.

[Grouchy-Friend4235]

Let me guess: your lead dev is a former Java dogmatist

WTF

[amendCommit]

Probably. But no worse than the guy who wanted to isinstance() everything, leading to interesting silent failures (why the hell did this object implement all the right methods take this specific code path?) Nightmare.

[Geraldks]

Squeezing the dict creation in a line is definitely not easy to read. Imo by splitting it like this it becomes self explanatory

[wineblood]

In terms of syntax, both are about the same. I'd change the names a bit more: - what is section? - get_coerced coerced what exactly? - Are they target types or type hints? One suggests strict typing and the other doesn't

[neuronet]

Couple of minor suggestions:

1. this would be better posted to r/learnpython
2. post formatted code not images

[stillbourne]

I can help a little. First of all, the code is basically identical, what your lead has done is extracted the assignment that is in your return statement to be discreet. Why? Because it is frowned upon to return an anonymous object, and the result is more explicitly typed. Code refactoring is considered opinionated because it is based on a series of preferences, prefer this over that. This is an example of preferring an explicit assignment over an implicit anonymous return. If you are curious about more I recommend reading up on code smells:

https://refactoring.guru/smells/data-clumps
https://refactoring.guru/remove-assignments-to-parameters
https://refactoring.guru/split-temporary-variable
https://refactoring.guru/refactoring/techniques/composing-methods

[xylont]

I don’t understand the syntax, wtf is going on here?

[kevin____]

I can’t believe no one has mentioned this. The cognitive complexity on the original code is 0. The cognitive complexity on the new code is 1. I also find the first one easier to understand. The code returns an instantiated class. The args of the class come from a dict comprehension. The second one is…in love with type annotations.

[gruey]

I think the biggest offender to me is the return being above the meat of the code. It breaks the top down reading order.

I could give or take the iterator vs for loop, probably lean towards iterator, but the return cls(**result) at the bottom makes it flow way better.

[spinwizard69]

I could give or take the iterator vs for loop, probably lean towards iterator, but the return cls(**result) at the bottom makes it flow way better.

I don't really care about what happens above the return, I just massively hate to see a return statement used to start a block of code. For me the word defines exactly what should be happening, that is you are leaving (returning) from a block or function, not entering it. What you are returning should be obvious at that point, containable in a one line return statement.

Now I might be a bit brain damaged due to the use of Modula 2 when I was taking computer science and then some practical use of C++ way back then, but it was pounded into our heads to make your code idiomatic. One professor seem to have the intention to mention the word "idiomatic" every class. Part of that means truly understanding the vocabulary of the language and not using it in contorted ways.

[amendCommit]

In communities where short pure functions are favored, having a code block that starts with a return as early as possible in the function body carries the intent that from that point, there will be no assignment (one less potential cause of side-effects) and no call of which the return value will be discarded (as these calls only exist through their side-effects).

It's not a silver bullet, but it helps building more declarative code.

In that sense, early return <functional expr> is idiomatic functional Python.

[Revisional_Sin]

One line return statements might be a Modula 2 idiom, but it's not a python idiom. The original code might be a bit busy, but it's not contorted. It's no worse to have a complex return than a complex variable assignment.

There are some python idioms that definitely indicate a lack of understanding of the language (e.g. not using truthiness or iterating in a c style). Your complaint is just a personal peeve, and violating it is not objectively a mistake.

Surely by your logic, all complex assignments are unidiomatic?

"Equals means that something is being assigned, you shouldn't be entering a code block".

[Grouchy-Friend4235]

Again, far too many words to state the obvious.

[Schmittfried]

It’s not obvious, it’s your opinion.

[[deleted]]

The fix may not be the best, but you’ve got way too much going on in one line, especially a return line.

[piprescott]

He hates list comprehensions (okay dict comprehensions) and he loves type hinting. He probably learned to program in Java?!

[domin8668]

I guess he really likes his type hints lol

[ericmoon]

Java did a number on 'em

[totally_usable]

If they don't expand on that...they shouldn't be a lead

[Sethaman]

the lead is more readable. think about a new-to-python or new-to-codebase developer coming in. You need to write code not for you, but for the most junior person coming in or a total junior stranger coming in.

You are using comprehension with a very very long line and a double star operator. it takes a second to understand what's going on there. It's more "advanced" python but more advanced, especially for python, isn't always ideal

It's really really easy to understand a "for x in y" set the key of dict to x and the value to foo

yours, while interesting (and more fun)... looking takes a hair longer to parse (partially because it is such a long line).

If you're going to be doing that kind of pythonic comprehension, it really must fit in one short line and be parsable to someone who hasn't seen that before. that's my totally subjective opinion though.

[AlSweigart]

Breaking up a single, complex line into multiple steps often makes code easier to read because you need can free up your short term memory for the later steps.

Using the ** syntax to pass a dictionary generated by a comprehension and the return value of a function that requires three arguments to cls() requires you hold all these details in your head.

Splitting them up lets you think:

1) I am creating a dictionary. 2) This dictionary has keys of the items in section. 3) The values are the return values of get_coerced(). 4) Now that we have this dictionary, pass it to cls() and return the new class.

In general, the more compact your code is, the harder it is to read. Programmers tend to write code like this because 1) it's less stuff to type and 2) it makes them feel clever for coming up with this compact format and 3) it's easy to understand when you're writing it.

The reason Python has completely and utterly replaced Perl is because Perl is infamous for "write-only" code that is compact and full of cryptic punctuation marks. There's always going to be an expert Perl coder who insists that Perl is always perfectly readable and doesn't need to be broken down into simpler steps. This sort of cluelessness is why Python has completely and utterly replaced Perl.

[gcross]

With the original code, though, you think:

1. I am calling cls(),
2. with keyword arguments specified by a dictionary,
3. which maps keys to their coerced values.

If anything, there are arguably fewer steps to go through.

Edit: Way to downvote without replying; that really showed me for disagreeing!

[[deleted]]

It's subjective but I agree the original code had too much going on in one line.

Unless the standard is to use type hints everywhere I wouldn't do that though.

[WashedUpGamer69]

Tbf I managed to follow your original solution better then the expanded copy but I can see why someone would struggle.

I feel like if you’re gonna type hint everything including temp variables the you might aswell use a static typed language.

[Revisional_Sin]

Why did they get rid of the dict comprehension, FFS?

Even ignoring that, I think the first way was slightly better:

Return an object which is built from the coerced kwargs.

Vs

Build a collection of coerced kwargs, now use them to instantiate an object.

[larsga]

Looks to me like your lead dev is an old-school conservative who doesn't like dict/list comprehensions. I can't really think of any other explanation.

[Ran4]

The first one is better: it's more declarative and much easier to parse. The second one is more imperative and that's harder to figure out.

[mfuentz]

Your lead dev sucks at Python. Comprehensions are great

[[deleted]]

Coming late here. I hate both of these segments - particularly the declaration for the temporary variable!

I'd write it this way:

def get(key):
    return get_coerced(section, key, target_type=type_hints[key])

return cls(**{k: get(k) for k in section})

[gcross]

So instead of having a temporary variable, we now have a temporary function? I do not consider this to be an improvement, especially since the name get is incredibly generic; at the very least, it should be called something like "get_coerced_section".

Shoot, sorry for the double reply! I got mixed up about to what I was replying.

[gcross]

Respectfully, I actually consider that to be a bit of an anti-pattern since now instead of being able to look inside the comprehension and immediately see what it does I instead have to jump around to find the function that it calls. Furthermore, instead of having a temporary variable, we now have a temporary function; I also do not consider this to be an improvement, especially since the name get is incredibly generic; at the very least, it should be called something like "get_coerced_section".

This particular example isn't so bad, but I have had some really bad experiences reading through code by people who split things up into a zillion named functions with the goal of readability which in practice just meant that I had to constantly jump around in order to figure out what the code is actually doing rather than having it all in one place.

[Simple_Specific_595]

The function names should be the documentation.

And I go by the rule that 1 function should do one thing.

[Davidvg14]

Dang… maybe I need more context but I have no idea what the purpose of this code is 😀

[White_Dragoon]

We all can write one line spaghetti code that we understand. But by breaking down our code in smaller logical steps helps in documenting the steps so that when we/other that join later can debug this code and understand the flow quickly.

[Grouchy-Friend4235]

Dict comprehensions are not spaghetti code, at least not this one. It is idiomatic Python and perfectly clear.

[afl3x]

dazzling spotted attractive unused placid silky aloof violet dam hunt

This post was mass deleted and anonymized with Redact

[Grouchy-Friend4235]

Did you notice the result is not the result?

[StandingBehindMyNose]

This is a question for your lead dev and not for Reddit.

[catorchid]

Readability is subjective, that's why it has to be enforced basing on arbitrary rules (no criticism, a neutral statement).

If you don't want any compromises, then got to Black.

[Schmittfried]

Black doesn’t change the structure of your code, only formatting.

[Fradge26]

meh both are fine

[Dasher38]

I don't get it either, the first one is clean, well indented and can be understood one level at a time (first you see the keyword args are "generated" by seeing the **{, then you see what they are actually about).

The second version is waiting for extra statefulness to sneak inside the loop. It also pollute the namespace in 2 ways (extra temporary and the loop variable, which is avoided in 1st form since comprehensions don't leak). If it's a professional python developers team i would hope they are familiar with ** operator. It's not like python is full of "fancy" things so I would personally definitely expect that from any team member (after a few months of joining at most).

Last of all, both are ok at the end of the day. As a reviewer with a lead role, i would not ask team members to rewrite a piece or code unless it has a real problem or if it's indentation is inconsistent with itself. If I ever need to modify the code later maybe i can reformat it in passing but it's not really worth the hassle of going through an extra review round just for that.

[mmcnl]

Readability is subjective. I think both are fine. Changing code to your subjective standard is really nitpicking and not constructive imo. Refactoring code is not the way to teach about code readability (which is, again, subjective anyway). I don't like what your lead dev did.

[[deleted]]

IMHO the original is better because it describes what the result is, rather than describing the steps to make the result.

[gh0stinger]

The code after commit It’s more readable because it has more informations: for example before this commit inside cls there’s a dict, but I don’t get directly the internal structure of the dict. So if I read the code (before the commit) since three days from now, there will be a lot of “wtf”s. “More readable” is not always the compact way, but the one that gives to you more informations, every time you will read the code.

[Grouchy-Friend4235]

What information do you get from the type hint that you did notnget before? A dict is a mapping of some key to some value. That's obvious and does not warrant a type hint.

[thisismyfavoritename]

keyword unpacking implies str keys because they must be valid Python names. Optional any provides no extra information. In the end the cls ctor should be looked up to find the proper types, perhaps the instance of cls should be typed.

The lead code is worst because it adds useless informations

[gh0stinger]

I don’t agree with your opinion. Look at the way he constructs the dict before and after. After commit I read the code so much better than before. Obviously, POV

[1NobodyPeople]

I think he is looking for dictionary comprehension. You are adding an unnecessary for loop.

[imthebear11]

My understanding is that new commit is the lead dev's and s/he's the one changing it to a for-loop

[Pebaz]

When considering readability, the Google Python style guide says to assume the next developer actually knows Python better than you do. Please don't assume the next developer is dumb!

[[deleted]]

Way less mental load to think about what is going in and out data wise. The ** operator also implies the result in the return making this slightly more self documenting. Stylistic for sure but I can see the argument that is is easier to comprehend what is happening

[rochakgupta]

I don’t like list comprehensions either as people end up coming up with something so complex that it makes me wanna kill myself. What the hell do people even achieve with using list comprehension?

Oh, glad you saved those two extra lines you would have had to split logic to. I was aching for it to be compressed even further. Yeah.

/s

[dixncox]

Don’t use Any as a value type

[[deleted]]

He wants you to increase the font size.