Better solution for this "if" spaghetti

Shell Programming and Scripting
1

Hello everyone,

I'm stuck and I need some help with rewriting the following code. It's ugly af and I'm not sure which direction to go.

err=0
trap 'err=1' ERR


# these 2x env. vars will be set by another script to either
# a value or to 'null' (4 chars)
TAG="${TAG}"
VALUE="${VALUE}"

# form the cmd to be executed
BASE_CMD="base_cmd arg1"
# if err=0, append "arg2"
SUCC_CMD="$BASE_CMD arg2"
# if err=1, change arg2 to arg3
FAIL_CMD="${SUCC_CMD//arg2/arg3}"
# if $TAG and $VALUE are set and err=0, do this:
SUCC_CMD2="$SUCC_CMD $TAG $VALUE arg4"
# same as above but if err=1
FAIL_CMD2="${SUCC_CMD2//arg4/arg5}"

# clear the 'null'
if [ "${AMI_TAG_KEY}" == 'null' ];then
        AMI_TAG_KEY=''
        AMI_TAG_VALUE=''
fi

# according to err, launch the cmds we created above
# assume $VALUE is null when $TAG is null
if [ "${err}" == "0" ] && [ ! -z "${TAG}" ];then
  $SUCC_CMD2
elif [ "${err}" == "0" ] && [ -z "${TAG}" ];then
  $SUCC_CMD
elif [ "${err}" != "0" ] && [ ! -z "${TAG}" ];then
  $FAIL_CMD2
  exit 1
elif [ "${err}" != "0" ] && [ -z "${TAG}" ];then
  $FAIL_CMD
  exit 1
fi

The idea is that I would like to avoid code duplicate (first segment) and then launch a specific command according to some logic (2nd and 3rd section). The only issue is that it looks ugly af to me and I'm sure there's a better way to rewrite it.

Any help or direction would be appreciated.

Thx in advance.

2

You might be better with a case statement. The structure is like this.

case "${err}" in
        0) echo "I saw value ${err}" ;;
        1) echo "I saw value ${err}"
            echo "This means something. ;;
        2) echo "I saw value ${err}"
            err=5        ;;
        *) echo "I saw something else - ${err} so....."
            echo "Panic" ;;
esac

I hope that this helps,
Robin

1 Like
3

I was thinking about a case statement but I have to evaluate $err and the value of $TAG together and I'm not sure how that would look and work. I guess something like this:

case "${err}" in
        "0")
        [ "${err}" == "0" ] && [ ! -z "${TAG}" ]
        $SUCC_CMD2
        ;;
        "1")
        [ "${err}" == "0" ] && [ ! -z "${TAG}" ]
        $FAIL_CMD2
        ;;
        *) printf "Something went wrong"
           exit 1
         ;;
esac
4

No case , no if , just shell "parameter expansion". May be not that easy to maintain in the long term (and uses the deprecated eval , but safely as the conditions are clearly defined):

eval "\$${W[err]:-ERROR}_CMD${TAG:+2}"

Example / test:

SUCC_CMD="echo SUCC1"
SUCC_CMD2="echo SUCC2"
FAIL_CMD2="echo FAIL2"
FAIL_CMD="echo FAIL1"
ERROR_CMD="echo SNAFU"
ERROR_CMD2="echo SNAFU"

W=(SUCC FAIL)

for err in 0 1 2; do for TAG in "" 1; do  eval "\$${W[err]:-ERROR}_CMD${TAG:+2}"; done; done
SUCC1
SUCC2
FAIL1
FAIL2
SNAFU
SNAFU
5

You have evaluated $err already so you don't need to repeat that!
I have highlighted what is always true and always false.
Your original spaghetti if-then-else-fi translated:

case $err in
0)
  if [ -n "$TAG" ];then
    $SUCC_CMD2
  else
    $SUCC_CMD
  fi
;;
*)
  if [ -n "$TAG" ];then
    $FAIL_CMD2
  else
    $FAIL_CMD
  fi
  exit 1
;; 
esac
1 Like
6

damn, that's a good one, dankeschoen.

BTW, regarding this:

# form the cmd to be executed BASE_CMD="base_cmd arg1" # if err=0, append "arg2" SUCC_CMD="$BASE_CMD arg2" # if err=1, change arg2 to arg3 FAIL_CMD="${SUCC_CMD//arg2/arg3}" # if $TAG and $VALUE are set and err=0, do this: SUCC_CMD2="$SUCC_CMD $TAG $VALUE arg4" # same as above but if err=1 FAIL_CMD2="${SUCC_CMD2//arg4/arg5}"

Does it make sense to keep it like this or change it to something else?

7

To construct variables from nother variables like

SUCC_CMD2="$SUCC_CMD $TAG $VALUE arg4"
FAIL_CMD2="${SUCC_CMD2//arg4/arg5}"

can go awfully wrong in some cases. Imaging $TAG or $VALUE contain "arg4"; is it intended to change these??
It is safer to repeat the previous

FAIL_CMD2="$SUCC_CMD $TAG $VALUE arg5"

And this will still go wrong in case $TAG or $VALUE contain a wildcard like *
Most safe is to have only one common static variable

BASE_CMD="base_cmd arg1"

and add the arguments in the branch structure:

case $err in
0)
  if [ -z "$TAG" ];then
    $BASE_CMD arg2
  else
    $BASE_CMD arg2 "$TAG" "$VALUE" arg4
  fi
;;
*)
  if [ -z "$TAG" ];then
    $BASE_CMD arg3
  else
    $BASE_CMD arg2 "$TAG" "$VALUE" arg5
  fi
  exit 1
;;
esac

And you more easily see logical errors.

1 Like
8

I understand. Thank you!