2
\$\begingroup\$

The following works fine and I'm not having to repeat any commands once logic is evaluated. However, it feels as though I'm doing a lot of seperate checks. I'm wondering if I could do this more efficiently?

#!/bin/bash

#The script can have 0, 1 or 2 input parameters
#parameter $1 can be one of 4 values ('foo','bar','chu','test')
#parameter $2 can only be 'test' and only if $1 is present and not 'test'


if ( [ $# == 1 ] && [ $1 == 'test' ] ) || ( [ $# == 2 ] && [ $1 != 'test' ] && [ $2 == 'test' ] ); then
    echo 'enable testing'
fi

if [ $# == 0 ] || ( [ $# == 1 ] && [ $1 == 'test' ] ); then
    echo 'do everything'
    exit 0
fi

if ( [ $# == 1 ] && [ $1 != 'test' ] ) || ( [ $# == 2 ] && [ $2 == 'test' ] ); then
    case "$1" in
        'foo')
            echo 'only do foo'
            exit 0
            ;;
        'bar')
            echo 'only do bar'
            exit 0
            ;;
        'chu')
            echo 'only do chu'
            exit 0
            ;;
        *)
            echo 'not valid'
            exit 1
    esac
else
    echo 'not valid'
    exit 1
fi

exit 0
\$\endgroup\$
2
\$\begingroup\$

You can achieve the same but simpler if you reorder the validation steps.

If there are two arguments and both are test, that's invalid:

[ "$1" = test ] && [ "$2" = test ] && invalid

Where invalid is a helper function:

invalid() {
    echo not valid
    exit 1
}

If the second argument is test, then enable testing, or else if the first argument is test then enable testing and shift the arguments:

if [ "$2" = test ]; then
    echo enable testing
elif [ "$1" = test ]; then
    echo enable testing
    shift
fi

At this point, the should be 0 or 1 arguments, not more. That was a missing validation step in the original code:

[ $# -le 1 ] || invalid

If we reach this point, there must be 0 or 1 arguments. The original case statement can come here as before, without any further if statements. It can use the invalid helper function in the invalid case.

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ Nice use of shift, this is exactly what I was after, thanks @janos \$\endgroup\$ – sansSpoon Sep 27 '17 at 1:07
1
\$\begingroup\$

One way to clean up this program is to separate the command line parsing from the rest of the program. I tried it, and my code looks like this:

#! /bin/bash
set -eu

usage() {
  echo "usage: $0 [foo|bar|cux] [test]" 1>&2
  exit 1
}

do_test=false
do_all=false
do_foo=false
do_bar=false
do_cux=false

case $#${1+,}${1-}${2+,}${2-} in
0)
  do_all=true
  ;;
1,test)
  do_test=true
  do_all=true
  ;;
1,*)
  # see below
  ;;
2,test,*)
  usage
  ;;
2,*,test)
  do_test=true
  ;;
*)
  usage
esac

case ${1-} in
foo) do_foo=true ;;
bar) do_bar=true ;;
cux) do_cux=true ;;
""|test) ;;
*) usage ;;
esac

# end of command line parsing

if $do_test; then echo 'in testing mode'; fi
if $do_all || $do_foo; then echo 'foo'; fi
if $do_all || $do_bar; then echo 'bar'; fi
if $do_all || $do_cux; then echo 'cux'; fi

At the end of the command line parsing, you have a clear state, consisting of only a few variables which you can print if you are unsure about their values.

If that's still too much code, there's another possiblity. Most programs that originated in UNIX parse the command line from left to right, and there is the handy shift command that discards arguments from the left.

So if you can swap the arguments and call the program with test foo instead of foo test, the code becomes much simpler:

#! /bin/bash
set -eu

usage() {
  echo "usage: $0 [test] [foo|bar|cux]" 1>&2
  exit 1
}

do_test=false
do_all=false
do_foo=false
do_bar=false
do_cux=false

if [ "${1-}" = "test" ]; then
  do_test=true
  shift
fi

case $#${1+,}${1-} in
0) do_all=true ;;
1,foo) do_foo=true ;;
1,bar) do_bar=true ;;
1,cux) do_cux=true ;;
*) usage ;;
esac

if $do_test; then echo 'in testing mode'; fi
if $do_all || $do_foo; then echo 'foo'; fi
if $do_all || $do_bar; then echo 'bar'; fi
if $do_all || $do_cux; then echo 'cux'; fi

The case statement may look frightening at first, but once you decompose it, it becomes a little easier to understand:

  • $# is the number of arguments.
  • ${1+,} is a , if command line argument number 1 is defined, and the empty string otherwise. The plus can be read as and.
  • ${1-} is the command line argument number 1 if it exists, and the empty string otherwise. The hyphen can be read as or.

The concatenation of all these parts then has one of the forms that are checked against in the case patterns. In essence, it is the command line as a single string, but with a comma as the separator. I did this so that the case patterns can be written without quotes.

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ Thanks @Roland. I haven't seen a case statement like that before $#${1+,}${1-}${2+,}${2-}. Could you please explain that a bit further? \$\endgroup\$ – sansSpoon Sep 27 '17 at 1:11
  • \$\begingroup\$ I explained it in the answer. \$\endgroup\$ – Roland Illig Sep 27 '17 at 15:47
  • \$\begingroup\$ Apologies, I missed the points at the bottom! \$\endgroup\$ – sansSpoon Sep 28 '17 at 3:42
  • \$\begingroup\$ No, you didn't miss it. I wanted to say "I just added an explanation" but your the wrong words. :-/ Thanks for your suggestion. \$\endgroup\$ – Roland Illig Sep 28 '17 at 3:58

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.