Make my set proxy script better

Question

Below is my bash script to setup a proxy, I am by no means an expert in bash. I suspect there is a better way to write this, any tips would be appreciated.

Is there a better way to do nested while-loops and if-statements?

#!/bin/bash
set -e    

#functions
function yesno() {
  while read -n1 -r -p "[y/n/QUIT] > "; do
    case $REPLY in
      y|n) return;;
      q) echo; echo; exit 1;;
      *) printf "\nERROR - Invalid response, please enter Yes OR No.\n";;
    esac
  done
}

function startover() {
  printf "\n\n\e[1m\e[31mStarting Over...\e[0m\n"
  sleep 2
  unset proxyport proxyhost
}


    shopt -s nocasematch
    while true; do
      printf "\nIs a proxy required to access outbound \e[1m\e[7mHTTP/HTTPS\e[0m sites?\n"
      yesno
      if [[ $REPLY == "y" ]]; then
        printf "\n\nPlease enter your proxy url\nExample - \e[1m\e[7mproxy.example.com:8080\e[0m\n"
        while read -r -p "Proxy > " proxy; do
          if [[ $proxy =~ ^https:// && $proxy =~ :[0-9]{1,5}$ ]]; then
            printf "\nERROR - HTTPS with a port is not supported, please re-enter without '\e[1m\e[97m\e[41mHTTPS://\e[0m' prefix\n"
          elif [[ $proxy =~ ^(https?://)?([a-z0-9][-a-z0-9\.]{0,61}[a-z0-9]\.[a-z]{2,5})?.((25[0-5]|2[0-4][0-9]|[01]?[0-9][0-9]?)\.(25[0-5]|2[0-4][0-9]|[01]?[0-9][0-9]?)\.(25[0-5]|2[0-4][0-9]|[01]?[0-9][0-9]?)\.(25[0-5]|2[0-4][0-9]|[01]?[0-9][0-9]?))?(:[0-9]{1,5})?$ ]]; then
            if [[ $proxy =~ :[0-9]{1,5}$ ]]; then
              proxyport=${proxy##*:}
              proxy=${proxy%:*}
              echo "Port entered $proxyport"
            elif [[ $proxy =~ ^https:// && ! $proxy =~ :[0-9]{1,5}$ ]]; then
              proxyport=443
              echo "URL starts with HTTPS, assuming $proxyport"
            else
              proxyport=80
              echo "No port entered, assuming $proxyport"
            fi
            proxyhost=${proxy##*/}
            break
          else
            printf "\nERROR - \e[1m\e[97m\e[41m$proxy\e[0m is not supported please try again\n"
          fi
        done
        printf "\nCreating Proxy Settings\n"
        sleep 2
        printf "Proxy Hostname: \e[1m\e[97m\e[44m$proxyhost\e[0m\nProxy Port: \e[1m\e[97m\e[44m$proxyport\e[0m\nDoes the information above look correct?\n"
        yesno
        if [[ $REPLY == "n" ]]; then
          startover
        fi
      fi
      break
    done
    shopt -u nocasematch

Update 4/6/17 I have improved the regex so it now supports IP and hostnames, i am still working on actually setting the proxy settings.

I'm no expert in bash either so I don't feel very qualified to post an actual answer. One thing I noticed is your case statement only allows for lowercase y n replies. I would consider making it [Yy]) ... and [Nn]) ... — I0_ol
– I0_ol, Commented Apr 6, 2017 at 0:21
And just to be nitpicky you are missing the shebang at the beginning of your script. — I0_ol
– I0_ol, Commented Apr 6, 2017 at 0:23
thanks for the input, since i am invoking 'shopt -s nocasematch' the user input is no longer case sensative, so Y and y will both match my case. — bc81
– bc81, Commented Apr 7, 2017 at 1:06

janos · Accepted Answer · 2017-04-18 18:05:16Z

Messy formatting

The first thing I noticed about this script is the messy formatting. Poorly formatted code is hard to read and maintain.

Extract complex conditions to helper functions

The code has many complex conditions. It would be more readable if you extracted them into helper functions.

For example, instead of this:

  if [[ $proxy =~ ^https:// && $proxy =~ :[0-9]{1,5}$ ]]; then
    # ...

You could introduce this helper function:

isHttpsWithPort() {
  [[ $proxy =~ ^https:// && $proxy =~ :[0-9]{1,5}$ ]]
}

And then use it with:

if isHttpsWithPort; then
  # ...

I recommend to change all complex conditions like that, and the code will be more readable, thanks to self-explanatory names of the functions.

Make `yesno` return success or failure

After calling the yesno function you check the value of REPLY. This is not very ergonomic. You could make yesno return success for "yes" and failure for "no":

yesno() {
  while read -n1 -r -p "[y/n/QUIT] > "; do
    case $REPLY in
      y) return 0;;
      n) return 1;;
      q) echo; echo; exit 1;;
      *) printf "\nERROR - Invalid response, please enter Yes OR No.\n";;
    esac
  done
}

When written this way, instead of this:

    yesno
    if [[ $REPLY == "n" ]]; then
      startover
    fi

You could simplify to:

yesno || startover

Similarly, instead of this:

  yesno
  if [[ $REPLY == "y" ]]; then

You could write:

if yesno; then

Loop not looping

The main while loop doesn't actually loop, so it should be removed:

while true; do
  # ...
  break
done

great input, i'll make the changes and update as I have added some additional changes since my last update — bc81
– bc81, Commented Apr 20, 2017 at 2:17

Stack Exchange Network

Make my set proxy script better

1 Answer 1

Messy formatting

Extract complex conditions to helper functions

Make `yesno` return success or failure

Loop not looping

You must log in to answer this question.

Hot Network Questions

Make my set proxy script better

1 Answer 1

Messy formatting

Extract complex conditions to helper functions

Make yesno return success or failure

Loop not looping

You must log in to answer this question.

Related

Hot Network Questions

Make `yesno` return success or failure