I had a piece of code that was deeply nested for loops, and I wanted to see what it could look like if it wasn’t deeply nested. Deep nesting is the root of all evil, you know. Which is the most clear, maintainable code?
Without deep nesting:
def filter_only_exists(lst: List[Path]) -> List[Path]:
"Filters a list of Path objects and returns a new list containing only the existing paths."
return [x for x in lst if x.exists()]
def find_playbook(playbookname: str) -> Path:
"Finds and returns the path of a specified playbook file."
search_path = os.environ.get(
"UP_PLAYBOOK_PATH",
".:.uplaybooks:~/.config/uplaybook/books:~/.config/uplaybook",
)
search_directories = [Path(x).expanduser() for x in search_path.split(":")]
extensions = ["", ".yaml", ".yml"]
pbdir_files = ["playbook.yaml", "playbook.yml", "playbook"]
paths_and_playbook_combinations = [
x[0].joinpath(playbookname + x[1])
for x in product(search_directories, extensions)
]
for playbook in filter_only_exists(paths_and_playbook_combinations):
if not playbook.is_dir():
return playbook
pbdir_combinations = [playbook.joinpath(x) for x in pbdir_files]
for pbdir in filter_only_exists(pbdir_combinations):
return pbdir
raise FileNotFoundError(
f"Unable to locate a playbook by the name of {playbookname},"
f" searched in path {search_path}"
)
Deep for loops version:
def find_playbook(playbookname: str) -> Path:
search_path = ".:.uplaybooks:~/.config/uplaybook/books:~/.config/uplaybook"
search_path = os.environ.get("UP_PLAYBOOK_PATH", search_path)
for dr in [Path(x).expanduser() for x in search_path.split(":")]:
for test in [dr.joinpath(playbookname + ext) for ext in ["", ".yaml", ".yml"]]:
if test.exists():
if test.is_dir():
for t2 in [
test.joinpath(x)
for x in ["playbook.yaml", "playbook.yml", "playbook"]
]:
if t2.exists():
return t2
return test
raise FileNotFoundError(
f"Unable to locate a playbook by the name of {playbookname},"
f" searched in path {search_path}"
)
Current Version
Tamás Gulácsi pointed out that in the Deep version above I really should be doing an “if not exists: continue”, which drops one level of nesting.
In thinking about this code, I decided to change the behavior that the above versions have: adding a suffix to the provided name, and then also treating it like a directory. So a “foo” playbook lookup could return “foo.yaml/playbook.yaml” (only if that path exists).
I decided I do like naming the more complicated list comprehensions, by first assigning them to a descriptive name (Chat GPT suggested that when I asked if it could make the code more obvious).
This is the version I ended up with and I really like it. The loop has become extremely simple, and could even be replaced with a “filter_exists()[0]”, but I think this is very clear. In particular, I find it very clear that I’m yielding an explicit set of name combinations. I could have made that a couple loops, but this I think is more obvious.
Chris McDonough replied that the deep loop was a more obvious read from top to bottom, which was what my gut was telling me (and why I titled this post “magic” in reference to the less deep version). My primary issue with that version was just the “code smell of deep nesting”.
So I’ll have to agree with Stephan Deibel who voted for “there’s got to be a better way”.
One note: I am throwing away a few syscalls here. I could put the first 4 yields behind a “if pb.exists()” or the top 3 behind a “if pb.is_dir()”. This function is only run once at startup though, so not a big optimization target.
def find_playbook(playbookname: str) -> Path:
search_path = os.environ.get(
"UP_PLAYBOOK_PATH",
".:.uplaybooks:~/.config/uplaybook/books:~/.config/uplaybook",
)
playbook_paths = [Path(x).expanduser().joinpath(playbookname)
for x in search_path.split(":")]
def playbook_combinations(paths: List[Path]) -> Iterator[Path]:
for pb in paths:
yield pb.joinpath('playbook.yaml') # pb/playbook.yaml
yield pb.joinpath('playbook.yml') # pb/playbook.yml
yield pb.joinpath('playbook') # pb/playbook
yield pb # pb
yield pb.parent.joinpath(pb.stem + '.yaml') # pb.yaml
yield pb.parent.joinpath(pb.stem + '.yml') # pb.yml
for pb in playbook_combinations(playbook_paths):
if pb.exists():
return pb
raise FileNotFoundError(
f"Unable to locate a playbook by the name of {playbookname},"
f" searched in path {search_path}"
)