2
\$\begingroup\$

I used this code to give -h(help) options to my bash script. It's working fine and I wanted to get the code reviewed.

while true; do
        if [ "$1" != "-l" ] && [ "$1" != "-a" ] && [ "$1" != "-h" ] && [ "$1" != "" ];
        then
                output=$(curl -k -s -u "$username:$password" -H "X-Requested-With: Curl" -d "action=list&echo_request=1" "${!url}" | grep -B 2 -w $name | grep ID | head -1 | sed 's/[^0-9]*//g')
        fi
        case "$1" in
                "")
                        echo "use -h for usage information";;
                -h)
                        echo  "-a       add"
                        echo  "-d       delete"
                        echo  "-l       name all"
                        echo  "-sv      addvlan"
                        echo  "-mv      add multiple vlan"
                        echo  "-dv      del vlan"
                        echo  "-ar      add route"
                        echo  "-mr      add multiple route"
                        echo  "-dr      delete route";;
                -a)
                        add ;;
                -d)
                        delete ;;
                -l)
                        list ;;
                -sv)
                        addvlan ;;
                -mr)
                        multipleroute ;;
                -dv)
                        delvlan "$@" ;;
                -mv)
                        addmulvlan ;;
                -dv)
                        delvlan ;;
                -ar)
                        addroute ;;
                -dr)
                        delroute "$@" ;;
        esac
        break
done;
\$\endgroup\$
  • \$\begingroup\$ I removed the sh tag. sh is a POSIX compliant shell, whereas bash (on most systems) is a superset of the POSIX shell. sh-scripts can run on bash, but bash scripts might not always run on a different implementation of the POSIX shell \$\endgroup\$ – Elias Van Ootegem Oct 19 '16 at 15:17
1
\$\begingroup\$

handling flags/arguments:

With bash scripts, it's more common to only use short flags (ie -h for help). In that case, you can use getopts:

while getopts hadl flag; do
    case $flag in
        h)
            help
            ;;
        a)
            # handle a
            ;;
        *) # the * "catchall" case is commonly used, arguably \? is better
            echo "unknonw option ${flag}" >&2
            help
            ;;
    esac
done

getopts flags can be digits, upper-case, and lower-case characters, meaning you have at least 62 distinct flags at your disposal. You've used a number of 2 digit flags, which is not supported, but looking at the list, you can easily replace them with single characters.
And even then, there's a lot of commands (eg ls) that handle duplicate short flags by assigning different behaviours to -a and -A.

With getopts, you can also tag option arguments. A basic example:

Usage() {
    echo "Usage: ${0##\*/}: -s [arg] -h"
    echo
    echo "    -s: Say whatever argument you pass to this flag"
    echo "    -h: Display this help message"
    exit "${1-0}" #defaul exit code
}

while getopts s:h flag; do
    case $flag in
        s)
            echo "You said ${OPTARG}"
            ;;
        h)
            Usage
            ;;
        *)
            echo "unknonw option ${flag} ${OPTARG}!" >&2
            Usage 1
            ;;
    esac
done

running ./script -h will display the usage stuff, and exit with status 0. Running ./script -s foobar will output You said foobar. Running ./script -q will tell you there's an unknown option ? q or something, display the help message and exit with status code 1.

If you run ./script -s without arguments, you'll get an error message saying s requires an argument. You'll then revert to the "unknown option" case, and see the help message and the script will terminate with status 1.

To prevent flags that take arguments from causing this error, prefix the flag with a colon:

while getopts :s:h flag; do

This will accept -s as a valid flag, even if the argument is missing.

An introduction to getops can be found here


Handling no flags/arguments

On closer inspection, I noticed this bit of code:

    case "$1" in
        "")
            echo "use -h for usage information";;
        -h)
            echo  "-a       add"
            ...

I'd say a more idiomatic, and easier way to detect whether or not the script was executed without arguments is to check the length of the argument array. Given my example script above, adding this before the while getopts:

[ "$#" -eq 0 ] && Usage 2

Ensures that, if $# (ie argument count) is 0, invoke the Usage function and exit with status 2. You could replace -eq with -le, but that really doesn't matter.

Alternatively, you can wrap the while getopts bit in a big if:

if [ "$#" -gt 0 ]; then
    while ...
    done
else
    Usage 2
fi

But the shorthand [ cond ] && action if cond true is very common, and easy enough to read anyway.


Wildcards/escaping

If you want to get a rough idea of the overall quality of your script, pass it through shellcheck. It will notify you if you forgot to escape variables that might introduce exploits and the like. Things like accepting bash wildcard characters as arguments, and then accidentally using them to run a command. Again, using the example script, let's change our s) case and try something:

s)
    ls $OPTARG
    echo "You said ${OPTARG}"
    ;;

Then run ./script -s "*". The script will run ls *, listing all sorts, and then echo "You said *". To prevent this from happening, the only thing you need to do is:

s)
    ls "${OPTARG}"
    echo "you said ${OPTARG}"
    ;;

And you'll probably get the output:

ls: cannot access *: No such file or directory
You said *

\$\endgroup\$
1
\$\begingroup\$

There is a break statement at the end of the while true. Like this, the while loop is completely pointless, you can remove it.

The output variable is never used. Either you forgot to include the rest of the script, or this is a pointless statement and should be removed.

The line that sets the output variable to the result of $(curl ...) is extremely long, and hard to read. It would be better to add some line breaks to reduce horizontal scrolling to read the code. The grep-grep-head-sed chain can be shorter too with a little Awk magic.

This long condition would be simpler to write with a case:

if [ "$1" != "-l" ] && [ "$1" != "-a" ] && [ "$1" != "-h" ] && [ "$1" != "" ]; then
    output=$(curl ...)
fi

Like this:

case "$1" in
    -l|-a|-h|"") ;;
    *)
        output=$(curl ...)
        ;;
esac
\$\endgroup\$
  • \$\begingroup\$ I didn't got ur point on \$\endgroup\$ – Sherry Oct 12 '16 at 21:45
  • \$\begingroup\$ case "$1" in -l|-a|-h|"") ;; *) output=$(curl ...) ;; esac \$\endgroup\$ – Sherry Oct 12 '16 at 21:45
  • \$\begingroup\$ I have to call a function on -l) if -l);; got executed then that function can't be called \$\endgroup\$ – Sherry Oct 12 '16 at 21:46
  • \$\begingroup\$ The case statement I wrote, is equivalent to your original if statement. Does exactly the same thing, but shorter to write. \$\endgroup\$ – janos Oct 13 '16 at 20:10
  • \$\begingroup\$ @janos Is there any reason in particular why you didn't suggest using getopts? \$\endgroup\$ – Elias Van Ootegem Oct 19 '16 at 14:34

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.