Is there any better way to repeatedly call function for each case statement-CodePudding

I wrote a bash script to automate lamp installing in my server, they work great but there's a problem regarding its readability.

  # Asking if want to install PHP or build from source
  read -p "This script will install PHP, do you want to install it (y/n)?" phpchoice
  case "$phpchoice" in
    [Yy]* ) isinstallphp=yes;;
    [Nn]* ) isinstallphp=no;;
    * ) echo "invalid";;
  esac

  if [ isinstallphp == "yes" ]; then
    # Prompt for php version
    options=("php5.6" "php7.0" "php7.1" "php7.2" "php7.3" "php7.4" "php8.0" "php8.1")
    function do_something () {
      echo -e "${Ye}You picked ${Wh}$php_opt${Ye}, will be installing that instead${Nc}"
    }
    select php_opt in "${options[@]}"; do
      case "$php_opt,$REPLY" in
        php5.6,*|*,php5.6) do_something; break ;;
        php7.0,*|*,php7.0) do_something; break ;;
        php7.1,*|*,php7.1) do_something; break ;;
        php7.2,*|*,php7.2) do_something; break ;;
        php7.3,*|*,php7.3) do_something; break ;;
        php7.4,*|*,php7.4) do_something; break ;;
        php8.0,*|*,php8.0) do_something; break ;;
        php8.1,*|*,php8.1) do_something; break ;;
      esac
    done    
  fi

The case statement part where I put do_something function looks messy, all the function do is echo which php option user choose in color. Is there any way to shorten the code ?

CodePudding user response：

The case statement part where I put do_something function looks messy, all the function do is echo which php option user choose in color. Is there any way to shorten the code ?

It appears that you are accommodating the dual possibilities that the user chooses an option by typing its number or by typing the item text. One simplification would be to require the user to make their selection by item number only, in which case you would only need to confirm that $php_opt was not set to NULL. The case statement would not be required at all.

If you want to retain the full input functionality of the current script, then you can still do better than the current code. Instead of a long case statement covering all the options, check whether $php_opt is NULL, and if it is, check whether $REPLY is equal to one of the options. There is a variety of ways you could implement that, but I like this one:

validate_option() {
  local choice=$1
  while [[ $# -gt 1 ]]; do
    shift
    [[ "$choice" = "$1" ]] && return 0
  done
  return 1
}

# ...

    select php_opt in "${options[@]}"; do
      if [[ -n "$php_opt" ]] || validate_option "$REPLY" "${options[@]}"; then
        do_something
      fi
    done

I find that a lot clearer. Note also that the validate_option function is reusable, and that this approach is completely driven by the option list, so you don't need to modify this code if the option list changes.

Addendum

You additionally raised a question about your given do_something function not printing the selected option when the user enters the option value instead of its number. This will have been a behavior of your original code, too. It arises from the fact that the select command sets the specified variable (php_opt in your case) to NULL in the event that the user enters a non-empty response that is not one of the menu item numbers.

If you want to avoid that, and perhaps also to have the selected value in the form of the option string for other, later processing, then you probably want to address that issue in the body of the select statement. Something like this variation might do, for example:

    select php_opt in "${options[@]}"; do
      if [[ -n "$php_opt" ]] ||
          { php_opt=$REPLY; validate_option "$php_opt" "${options[@]}"; }; then
        do_something
      fi
    done

That copies the user-entered text into $php_opt in the event that the user entered something other than an option number. Do note that that behavior change might have other effects later in the script, too.

CodePudding user response：

What you're checking is if $php_opt or $REPLY is in the $options array:

array_contains() {
  local -n ary=$1
  local elem=$2
  local IFS=$'\034'
  [[ "${IFS}${ary[*]}$IFS" == *"${IFS}${elem}$IFS"* ]]
}

#...
    select php_opt in "${options[@]}"; do
      if array_contains options "$php_opt" || array_contains options "$REPLY"; then
        do_something
        break
      fi
    done

local -n requires bash v4.3

Octal 034 is the ASCII "FS" character, it's unlikely to be in your data

CodePudding user response：

select sets the given variable name to the selected string, only if a valid selection is made. If an invalid selection is made (ie. not one of the available numbers), $php_opt will be empty. Even if a valid selection was made in a previous iteration, it will be reset to empty.

Hence you just need to test if $php_opt is empty or not:

select php_opt in "${options[@]}"; do
    if [[ "$php_opt" ]]; then
        do_something
        break
    else
        echo "$REPLY: invalid selection"
    fi
done

Or with case:

select php_opt in "${options[@]}"; do
    case $php_opt in
        '') echo "$REPLY: invalid selection";;
         *) do_something; break
    esac
done

From help select:

If the line consists of the number corresponding to one of the displayed words, then NAME is set to that word. If the line is empty, WORDS and the prompt are redisplayed. If EOF is read, the command completes. Any other value read causes NAME to be set to null. The line read is saved in the variable REPLY.

Also note that you are missing $ in your if statement.