1

I am very new to Bash shell scripting, and I finally got the following code working. I couldn't make it work any other way, but I feel like it is is very sloppy code.

Can someone please help me re-write this code in idiomatic (safe, concise, easy-to-read) Bash? I am wondering particularly if there is a better way to return the value from the function, and also if I am testing it correctly in the if statement. (Should I use brackets for the if statement?)

#!/bin/bash

input() {
    read -p $'\e[31m\e[1m'"$1"$' [Y/n] \e[0m' -n 1 -r
    echo
    if [[ $REPLY =~ ^[Yy]$ ]]
    then
        return 0
    fi
    return 1
}

if input "Upgrade Arch?"
then
    sudo pacman -Syu
fi
3
  • 2
    The sloppy thing is assuming that it's always running in a terminal with color support. Otherwise you could replace the whole if [[ .. ]]; ... fi; return 1 with just [[ $REPLY = [Yy] ]]. – mosvy Feb 29 '20 at 13:59
  • 3
    Simply as a comment from a random person reading the code, "input-is-yes" sends a clearer signal about the function's intention. – Jeff Schaller Feb 29 '20 at 14:14
  • You could switch to zsh which has read -q for that. – Stéphane Chazelas Jun 5 '20 at 16:15
5

You can simply end a function in a statement, then the return code of the function will be that of the statement itself.

The check [[ $REPLY =~ ^[Yy]$ ]] looks good. Since you're using the bash built-in double square bracket test, you don't need to quote your variables, since word splitting doesn't happen inside them. Using the regex check is nice, though the simpler glob match with [[ $REPLY = [Yy] ]] also works.

I guess the main other change I'd make would be to use a local variable in your function and pass it to read. You can mark it at local or use declare which makes variables local when used inside a function.

One last bit is that you might want to consider enabling options errexit (-e), nounset (-u) and pipefail in your scripts to make them safer. That would prevent the script from continuing after a failed command and would help to catch typos in variable names. You need to code more defensively in those cases (explicitly testing for error in most of your commands, using defaults for environment variables that may be unset) but it makes for easier debugging when something unexpectedly breaks.

Putting it all together:

#!/bin/bash

set -eu -o pipefail

input() {
    declare confirm
    declare -r prompt=${1:-"Confirm?"}
    read -p $'\e[31m\e[1m'"${prompt}"$' [Y/n] \e[0m' -n 1 -r confirm
    echo
    [[ $confirm =~ ^[Yy]$ ]]
}

if input "Upgrade Arch?" ; then
    sudo pacman -Syu
fi

And as @StéphaneChazelas suggests, you could even further improve this to:

set -eu -o pipefail

input() {
    declare confirm
    declare -r prompt=${1:-"Confirm?"}
    IFS= read -p $'\e[31;1m'"${prompt}"$' [Y/n] \e[m' -n 1 -r confirm
    echo >&2
    [[ $confirm = [Yy] ]]
}

if input "Upgrade Arch?" ; then
    sudo pacman -Syu
fi
3
  • @StéphaneChazelas True! I mostly kept that from the OP since it wasn't at the core of the question... I'll still update the answer, thanks! – filbranden Jun 5 '20 at 16:38
  • @StéphaneChazelas Done! Please take a look, feel free to further edit the answer if you feel like the script could be further improved. – filbranden Jun 5 '20 at 20:08
  • 1
    Good. I'll just leave for the record that not everybody would agree that set -e or readonly/declare -r are recommended good practice. – Stéphane Chazelas Jun 5 '20 at 20:27

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.