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.

• I0_ol: 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: And just to be nitpicky you are missing the shebang at the beginning of your script.
• bc81: 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.

Answer 1

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

• bc81: great input, i'll make the changes and update as I have added some additional changes since my last update