From: Florian Gamböck Date: Mon, 09 Apr 2018 13:36:27 GMT Subject: Re: [RFC PATCH 1/1] completion: load completion file for external subcommand Message-ID: <20180409133627.GA25078@arbeitsviech> In-Reply-To: On 2018-04-09 18:36, Junio C Hamano wrote: > Florian Gamböck writes: > > > Good point. I could go even further and ditch the if-construct: > > > > ! declare -f $completion_func && declare -f __load_completion && > > __load_completion "git-$command" > > I personally find that a lot harder to read than if/then/fi. Then I mis-understood you the first time. It sounded a bit as if you wanted to avoid if-fi. After all, the rest of this code block also uses quite long &&-chains. Then we are back at the first question: > > local completion_func="_git_${command//-/_}" > > + if ! declare -f $completion_func >/dev/null 2>/dev/null; then > > + declare -f __load_completion >/dev/null 2>/dev/null && > > + __load_completion "git-$command" > > wouldn't the above be easier to read if it were > > if ! declare ... $completion_func ... && declare -f __load_completion > then > __load_completion "git-$command" > fi > > or is there a reason why it is better to &&-chain the check for > __load_completion with its use? As for readability, I would prefer my first approach then with the following reasoning: Checking the existence of a function and actually calling it can be seen as a unit. Either the function does not exist or you call it. You could even create a function like "call_if_exists", that does exactly this. Either way, at the end of the line you are smarter than before. As for the if-statement, this describes the reason why you even want to load an external file. If the function in question ($completion_func) already exists, we do not want to load it again. If you chain the statement with the existence check of __load_completion, you make it look like those two functions are somehow related, which is not the case. To put it in words: "If $completion_func does not already exist, then load another file (if you know how to do that)." versus "If $completion_func does not yet exist and you know how to load another file, then load another file." The difference is subtle, but I think the first sentence better describes the intention. Does my reasoning make sense? I mean, the result will be exactly the same, we are clearly only talking about readability here.