0
\$\begingroup\$

Staying true to the mantra: "The third time you do something, you automate", I've built up a decent collection of shell scripts that live in ~/bin. Some of these scripts contain useful boilerplate code, or just stuff I can't bring myself to memorise (like colour output stuff). Other scripts I find myself elaborating on from time to time. Though not a great pain, I am very lazy. Rather than typing vim ~/bin/some_script, I've written yet another script that expands some_script to its full path (using which), and opens it in vim. I'm perfectly happy with how the script works, and added a few basic creature-comforts, like checking if any (valid) arguments were provided and so on. Truth be told, I've not been writing that much bash in recent years, and some of the expressions/tricks I found myself using feel clunky. I know I used to have something like this in my .profile, but that particular incarnation of the script/function never made it to my github misc repo.

In short, I'd like to know if there's some stuff I could improve on. First the code, then some specific things that just don't feel right to me.

Usage() {
    cat <<-__EOF_
${0##*/} finds executables in PATH and opens them in vim - useful to quickly edit your scripts
    -o         : opens all scripts using o flag (equivalent to vim -o <paths>)
    -O         : similar to -o, but uses -O flag (so vim -O <paths>)
    -h         : Display help message

__EOF_
    exit "${1:-0}"
}

flags=""
paths=""

while getopts :oOh f; do
    case $f in
        o)
            flags="-o"
            ;;
        O)
            flags="-O"
            ;;
        h)
            Usage
            ;;
        *)
            echo "Unknown flag ${f}${OPTARG}"
            Usage 1
            ;;
    esac
    shift
done

[ $# -lt 1 ] && echo "No arguments given" && Usage 2

for b in "$@"; do
    p=$(which "${b}") && paths="${paths} ${p}"
done
[ ${#paths} -gt 0 ] || :(){ echo "No valid executables found in path" && exit 3; };:

# We want globbing and word splitting here
# shellcheck disable=SC2086
vim $flags $paths

Things that just feel a tad off would be:

  • [ $# -lt 1 ] && echo "No arguments given" && Usage 2 I do this all the time, and it works just fine, but chaining together 3 commands with && like this just looks ugly. If there's a cleaner, more concise way to do the same thing, I'm all ears.
  • p=$(which "${b}") && paths="${paths} ${p}". I'm not sold on the use of the subshell here. I'm using #!/usr/bin/env bash for my hashbang, I'm not sure I need the subshell. If not, what would be the alternative? What are the pro's, what are the con's?
  • [ ${#paths} -gt 0 ] || :(){ echo "No valid executables found in path" && exit 3; };: same as chaining things, only this time, because I'm using an ||, then an &&, I just wrap the last 2 expressions in a function and call it. This smells IMO. I could change it to [ ${#paths} -le 0 ] && ..., but then it's just another chain. Personally, I prefer -gt over -le or -eq in this case. I also prefer to have the eventual command (vim) to be the last one, so that the exit code of vim is passed back on success (without needing an explicit exit like [ ${#paths} -gt 0 ] && vim $flags $paths && exit or exit $? if want to be super-explicit)

There might be other things I can improve on, so feel free to point them out.

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

Use a shebang

#!/usr/bin/env bash

I see you're already doing that.

In bash, prefer [[...]] over [...]. The double bracket conditional command gives you more features (including regular expression matching), and fewer surprises (particularly about unquoted variables).

With a getopts loop, I prefer to shift at the end:

while getopts ...; do
    case $opt in
        ...
    esac
done
shift $((OPTIND - 1))

Allow the Usage function to take an error message:

Usage() {
    [[ -n $2 ]] && echo "$2" >&2
    # ... rest is the same
}

That simplifies some assertions

(( $# > 0 )) || Usage 2 "No arguments given"

Use command -v instead of which

Use an array to hold the paths. There's no other good way to hold a list of things where any of them might contain whitespace.

paths=()
for b in "$@"; do
    p=$( command -v "$b" ) && paths+=("$p")
done

You do need to command substitution (aka subshell) so you can capture the output.

The "colon" function makes me cringe. For clarity, use if instead of long boolean chains

if (( ${#paths[@]} == 0 )); then
    echo "No valid executables found in PATH" >&2
    exit 3
fi

And prefer quoting over word splitting:

vim $flags "${paths[@]}"

Yes, the script will exit with vim's exit status. If you want to be super tidy, you could replace the script process with vim:

exec vim $flags "${paths[@]}"
\$\endgroup\$
4
  • \$\begingroup\$ Thanks for the input. The colon function felt dirty to write indeed. Just out of curiosity: any specific reason to use (()) for numeric comparisons over [ -eq ] & co? I get why one would prefer using > of -gt, but oddly enough, I find myself making less silly mistakes using the latter (I probably mess up < vs > at least once a week - embarrassing, I know) \$\endgroup\$ Mar 25 at 17:55
  • \$\begingroup\$ Just a personal preference. ((...)) allow you to assign variables within it , and also to refer to variables without $ to create readable C-like expressions -- ((sum += hours * hourly_rate)) \$\endgroup\$ Mar 25 at 18:34
  • \$\begingroup\$ ((...)) does not play well with set -e though: if the arithmetic expression evaluates to zero, the return status is 1 -- makes it good for conditions, but can be confusing source of errors. \$\endgroup\$ Mar 25 at 18:36
  • \$\begingroup\$ cheers. In that case, I'll stick to the square brackets for bash scripts. Implemented pretty much all of the other suggestions you made. \$\endgroup\$ Mar 25 at 19:04

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.