Why is testing "$?" to see if a command succeeded or not, an anti-pattern?

17
votes

I see here that testing whether $? is zero (success) or something else (failure) is an anti-pattern, but I have not been able to find this anywhere else.

Sticking to the definition of anti-pattern of the Wikipedia: "An anti-pattern (or anti-pattern) is a common response to a recurring problem that is usually ineffective and risks being highly counterproductive." Why would this be an anti-pattern?

2
bashshellsh
"Usually ineffective" is a poorly-thought-through component of that definition. Something can be an antipattern while working more than 50% of the time, if there's a best-practice alternative that addresses a major pitfall or caveat in same without any negative tradeoff. Indeed, several things that are antipatterns are such because they introduce security risks; a security bug only needs to be exploited once to make the code with that vulnerability unacceptable, not on more than 50% of executions. – Charles Duffy
...unless it means "usually" in that most, but not all, uses of antipatterns tend to be ineffective; that's fuzzier, and thus more difficult to dispute. – Charles Duffy
See BashPitfalls #44 (cmd; (( ! $? )) || die). – pjh

2 Answers

29
votes

This is an antipattern because it introduces complexity that wouldn't exist if you didn't require the exit status to be recorded at all.

if your_command; then ...

has much less to go wrong than

your_command
if [ "$?" -eq 0 ]; then ...

For examples of things that can go wrong: Think about traps, or even new echo statements added for debugging, modifying $?. It's not visually obvious to a reader that a separate line running your_command can't have anything added below it without changing logical flow.

That is:

your_command
echo "Finished running your_command" >&2
if [ "$?" -eq 0 ]; then ...

...is checking the echo, not the actual command.

Thus, in cases where you really do need to deal with exit status in a manner more granular than immediately branching on whether its value is zero, you should collect it on the same line:

# whitelisting a nonzero value for an example of when "if your_command" won't do.
your_command; your_command_retval=$?
echo "Finished running your_command" >&2 ## now, adding more logging won't break the logic.
case $your_command_retval in
  0|2) echo "your_command exited in an acceptable way" >&2;;
  *)   echo "your_command exited in an unacceptable way" >&2;;
esac

Finally: If you enclose your_command inside of an if statement, this marks it as tested, such that your shell won't consider a nonzero exit status for purposes of set -e or an ERR trap.

Thus:

set -e
your_command
if [ "$?" -eq 0 ]; then ...

...will never (barring a number of corner cases and caveats which plague set -e's behavior) reach the if statement with any value of $? other than 0, as the set -e will force an exit in that case. By contrast:

set -e
if your_command; then ...

...marks the exit status of your_command as tested, and so does not consider it cause to force the script to exit per set -e.

0
votes

In my opinion this is an anti pattern if you don't need return value of the command and just need to check status.

If we need function return value and exit code there is no other way (or i don't know it, correct me if i am wrong). In example below we need to proceed only if previous command was succesfull and we are using return value in other function

Example:

test() { echo "A" ; return 1; }
l_value=$(test); l_exit_code=$?;

if (( l_exit_code == 0 )); then
   fn_other_function_to_pass_value $l_value
else
   echo "ERROR" >&2
   exit 1
fi