3
\$\begingroup\$

I wrote this code:

PROJECTS_LIST = [
    project if not project.update({"project_id": project_id}) else None for project_id, project in PROJECTS.items()
]

where PROJECTS is a dict. The goal is to convert a dict like {"project123": {"a": "b"}} to [{"project_id": "project123", "a": "b"}]

I worry that this isn't the best approach.

\$\endgroup\$

3 Answers 3

6
\$\begingroup\$
project if not project.update({"project_id": project_id}) else None

The above section is rather odd. I would prefer to one of the following solutions depending on Python version:

  • Python 3.9+: use the dictionary merging operator |:

    project | {"project_id": project_id}
    
  • Before Python 3.9: The, in my opinion, nicest alternate was to use dictionary unpacking:

    {**project, "project_id": project_id}
    

I'd recommend having a newline before the for, but otherwise the code looks fine.

PROJECTS_LIST = [
    project | {"project_id": project_id}
    for project_id, project in PROJECTS.items()
]
\$\endgroup\$
0
5
\$\begingroup\$

I agree with @Peilonrayz that a "constructive" functional approach such as using the | operator is the way to go for a list comprehension. Comprehensions shouldn't really be about side effects -- that's not how human minds view them, independent of what the machine does.

But let's accept that .update() shall be used, and it will be used by a comprehension. Then instead of the cryptic

... = [
    project if not project.update({"project_id": project_id}) else None
    for project_id, project in PROJECTS.items()
]

I feel this is far more clear:

... = [
    project.update({"project_id": project_id}) or project
    for project_id, project in PROJECTS.items()
]

We have an "A or B" disjunct, where we evaluate A for side effects, and then since it is guaranteed to return None the short-circuiting or operator moves along and returns the (updated) project value.

The original formulation had an awkward else clause in order to satisfy syntax rules.


If we're evaluating for side effects, a list comprehension is not a natural way to accomplish that.

Prefer:

for project_id, project in PROJECTS.items():
    project.update({"project_id": project_id})

and subsequently use a comprehension to read out the results.

\$\endgroup\$
3
\$\begingroup\$

I worry

A little worry is a good thing because it spurs you into action, like posting a question here on Code Review :)

Approaches to alleviate your worry:

  • Add documentation to your code (comments/docstrings) to more clearly specify the PROJECTS input dict
  • Add documentation to more clearly specify the PROJECTS_LIST output
  • Add tests to check a variety of inputs
  • Make sure the tests hit the else clause

I assume the PROJECTS input you showed is a minimal example. Make sure to test more realistic input, with multiple projects and multiple key/values within a project (if that is supported).

Here is another possible input. I split the long line because long lines are hard to understand and maintain.

PROJECTS = {
    "project123": {"a": "b"},
    "project45" : {"c": "d"},
    "project6"  : {"e": "f", "this" : "that"}
}

PROJECTS_LIST = [
    project if not project.update({"project_id": project_id}) else None
        for project_id, project in PROJECTS.items()
]

print(PROJECTS_LIST)
\$\endgroup\$
0

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.