Work in progress
Useless accesses to parent_dict
You iterate over the keys of parent_dict
and then retrieve parent_dict[key]
in various places. For a start, you could use a variable to do it just once but there are other (better) options.
Indeed, you could iterate over both keys and values in one go:
def get_children_if_parent(tag: str, parent_dict: dict) -> list:
tag_list = []
for key, val in parent_dict.items():
if type(val) == dict:
if key == tag:
target = ''
else:
target = tag
tag_list.extend(get_children_if_parent(target, val))
else:
if tag == key or tag == '':
tag_list.extend(val)
elif tag in val:
tag_list.append(tag)
return tag_list
Reduce indentation depth with elif
You could write the inside of the loop as:
if type(val) == dict:
if key == tag:
target = ''
else:
target = tag
tag_list.extend(get_children_if_parent(target, val))
elif tag == key or tag == '':
tag_list.extend(val)
elif tag in val:
tag_list.append(tag)
Edit:
Smaller functions
My feeling is that the logic around the empty string makes the logic harder to understand. Also, having an arbitrary value act as a flag may be misleading for the user who may genuinely want to use ""
as a argument. It may be easier to define a short function handle the case where key == tag
and val
is a dict.
We'd get something like:
def get_all_children(parent_dict: dict) -> list:
tag_list = []
for key, val in parent_dict.items():
if type(val) == dict:
tag_list.extend(get_all_children(val))
else:
assert type(val) == list
tag_list.extend(val)
return tag_list
def get_children_if_parent(tag: str, parent_dict: dict) -> list:
tag_list = []
for key, val in parent_dict.items():
if type(val) == dict:
if key == tag:
tag_list.extend(get_all_children(val))
else:
tag_list.extend(get_children_if_parent(tag, val))
else:
assert type(val) == list
if tag == key:
tag_list.extend(val)
elif tag in val:
tag_list.append(tag)
return tag_list
Second edit
Relaxing constraints on get_all_children inputs
By-reading the code above, something appeared to me: in 2 different places, we do the same thing, we get either get_all_children(val)
or just val
based on whether val
is a list.
It could be easier to just accept that get_all_children
can accept a list a as parameter and the logic becomes:
def get_all_children(collection) -> list:
if type(collection) == list:
return collection
assert type(collection) == dict
tag_list = []
for key, val in collection.items():
tag_list.extend(get_all_children(val))
return tag_list
Now, in get_children_if_parent
, we can handle key == tag
similarly for different types of input.
We'd get something like:
def get_children_if_parent(tag: str, parent_dict: dict) -> list:
tag_list = []
for key, val in parent_dict.items():
if key == tag:
tag_list.extend(get_all_children(val))
elif type(val) == dict:
tag_list.extend(get_children_if_parent(tag, val))
else:
assert type(val) == list
if tag in val:
tag_list.append(tag)
return tag_list
But then I realized that maybe the same strategy could be applied here: if get_children_if_parent
accepted both lists and dicts, the implementation may be more straight-forward:
def get_children_if_parent(tag: str, collection) -> list:
if type(collection) == list:
return [tag] if tag in collection else []
assert type(collection) == dict
tag_list = []
for key, val in collection.items():
tag_list.extend(get_all_children(val) if key == tag else get_children_if_parent(tag, val))
return tag_list
isinstance
instead oftype
:if isinstance(val, dict):
. You can also assigntarget
using a conditional expression:target = '' if key == tag else tag
. \$\endgroup\$