Code review comment for ~ycheng-twn/checkbox-ng:0001_checkbox_cli_show

Revision history for this message
Maciej Kisielewski (kissiel) wrote :

Thank you for all the improvements!

I know I should have asked this earlier, but why only some parts of the records are printed?
Printing all of them would be so much simpler and possibly would carry more information with it.

I also found possible problems/room for improvement in the command's positional argument(s) handling.
If the code requires an argument then just remove the `nargs='?'` from the add_argument invocation. The whole branch from the invoked() method can go away as argparse would cover it for you. Also, if calling that command without an argument is wrong then it should not return None (which ends up being return code of 0).

As for the nargs, I'd prefer to see '+' there, as it means there must be at least one, while giving the user option to specify more than one name. This would be stored as a list so it'd be super easy to iterate over.

`attr1` and `attr2` don't really communicate their purposes.

The recursive part is not trivial and could use some comment or a docstring.

IMHO the search_and_show function has too many responsibilities:
* it defines the "filters" - which are constant, but are defined in a function that's called recursively, which introduces unnecessary performance penalty
* it understands what the object it searches for is
* traverses recursively the object tree
* it calls the printing function (increasing the complexity) and making it harder to test (maybe that's why there's no tests in this MR)

IMHO much clearer (and less complex) structure would be following set of functions and their respective responsibilities:

print_concrete_job(obj) - self explanatory
print_template() - self explanatory
find_object(obj_name) - traverses the object tree and returns the object with given name

Taken into consideration the `nargs='+'` advice the find_object could take an iterable of names enabling the name check to work on the whole list (which would be faster than searching for object while iterating over list of names - recursive tree traversal is much more expensive than checking if string is in a relatively short list of strings)

« Back to merge proposal