Skip to main content
Unix & Linux

Return to Answer

added 33 characters in body
Source Link
cas
cas
  • 84.7k
  • 9
  • 138
  • 206

The main problem with your script is a boolean logic error. In your elif clause you are testing for ("not cli OR not gui") when you should be testing for ("not cli AND not GUI"gui").

One last thing, you're using essentially the same echo -e statement twice. If that's something you're likely to use a lot onin your script, you should turn it into a function. This will make your script more consistent, and easier to read. Also easier to make changes, e.g. if you want to change the colour for all error messages, or add a timestamp, or whatever.

The main problem with your script is a boolean logic error. In your elif clause you are testing for ("not cli OR not gui") when you should be testing for ("not cli AND not GUI").

One last thing, you're using essentially the same echo -e statement twice. If that's something you're likely to use a lot on your script, you should turn it into a function. This will make your script more consistent, and easier to read. Also easier to make changes, e.g. if you want to change the colour for all error messages.

The main problem with your script is a boolean logic error. In your elif clause you are testing for ("not cli OR not gui") when you should be testing for ("not cli AND not gui").

One last thing, you're using essentially the same echo -e statement twice. If that's something you're likely to use a lot in your script, you should turn it into a function. This will make your script more consistent, and easier to read. Also easier to make changes, e.g. if you want to change the colour for all error messages, or add a timestamp, or whatever.

better explanation of boolean logic error. added truth table. added error_exit fn example with tput and printf.
Source Link
cas
cas
  • 84.7k
  • 9
  • 138
  • 206

The main obvious problem with your script is a boolean logic error. In your elif clause you are testing for ("not cli"cli OR "notnot gui") when you should be testing for ("not cli"cli AND "notnot GUI").

With the OR, the test failssucceeds if the variable doesn't match either "cli" or "gui". With, so half of the results will be incorrect. With AND, it failssucceeds only when it doesn't match both.

This is shown in the truth table below. "NOT cli & "NOT gui" are the conditions being tested, while the AND and OR columns show the results of AND-ing and OR-ing the first two columns.

NOT cliNOT guiANDOR
falsefalsefalsefalse
falsetruefalsetrue
truefalsefalsetrue
truetruetruetrue

However, that can be improved because regular expressions support alternations with the | pipe character, so you only need to make one regex test. e.g.

shopt -s nocasematch if [[ ! $interfaceType =~ ^(cli|gui)$ ]]; then echo -e "[${c_RED}ERROR${c_RESET}] the interface type must be 'cli' or 'gui'" && exit 2 fi

One last thing, you're using essentially the same echo -e statement twice. If that's something you're likely to use a lot on your script, you should turn it into a function. This will make your script more consistent, and easier to read. Also easier to make changes, e.g. if you want to change the colour for all error messages.

For example (using tput rather than hard-coding ANSI/vt-100 colour codes, and using printf rather than echo -e, see Why is printf better than echo? - in short, echo -e is non-portable and unreliable):

#!/bin/bash shopt -s nocasematch c_RESET=$(tput sgr0) # turn off all attributes c_RED=$(tput bold setaf 1) # bold red error_exit () { # first arg is the exit code ec=$1 shift # Remaining args ($*) are printed with a red ERROR message. printf "[${c_RED}ERROR${c_RESET}] %s\n" "$*" exit $ec } interfaceType="$1" if [ -z "$interfaceType" ]; then error_exit 1 "The interface type must be specified. (cli/gui)" elif [[ ! $interfaceType =~ ^(cli|gui)$ ]]; then error_exit 2 "The interface type must be 'cli' or 'gui'" fi echo $interfaceType

The main obvious problem with your script is a logic error. In your elif clause you are testing for ("not cli" OR "not gui") when you should be testing for ("not cli" AND "not GUI").

With the OR, the test fails if the variable doesn't match either "cli" or "gui". With the AND, it fails only when it doesn't match both.

However, that can be improved because regular expressions support alternations with the | pipe character. e.g.

shopt -s nocasematch if [[ ! $interfaceType =~ ^(cli|gui)$ ]]; then echo -e "[${c_RED}ERROR${c_RESET}] the interface type must be 'cli' or 'gui'" && exit 2 fi

The main problem with your script is a boolean logic error. In your elif clause you are testing for ("not cli OR not gui") when you should be testing for ("not cli AND not GUI").

With OR, the test succeeds if the variable doesn't match either "cli" or "gui", so half of the results will be incorrect. With AND, it succeeds only when it doesn't match both.

This is shown in the truth table below. "NOT cli & "NOT gui" are the conditions being tested, while the AND and OR columns show the results of AND-ing and OR-ing the first two columns.

NOT cliNOT guiANDOR
falsefalsefalsefalse
falsetruefalsetrue
truefalsefalsetrue
truetruetruetrue

However, that can be improved because regular expressions support alternations with the | pipe character, so you only need to make one regex test. e.g.

shopt -s nocasematch if [[ ! $interfaceType =~ ^(cli|gui)$ ]]; then echo -e "[${c_RED}ERROR${c_RESET}] the interface type must be 'cli' or 'gui'" && exit 2 fi

One last thing, you're using essentially the same echo -e statement twice. If that's something you're likely to use a lot on your script, you should turn it into a function. This will make your script more consistent, and easier to read. Also easier to make changes, e.g. if you want to change the colour for all error messages.

For example (using tput rather than hard-coding ANSI/vt-100 colour codes, and using printf rather than echo -e, see Why is printf better than echo? - in short, echo -e is non-portable and unreliable):

#!/bin/bash shopt -s nocasematch c_RESET=$(tput sgr0) # turn off all attributes c_RED=$(tput bold setaf 1) # bold red error_exit () { # first arg is the exit code ec=$1 shift # Remaining args ($*) are printed with a red ERROR message. printf "[${c_RED}ERROR${c_RESET}] %s\n" "$*" exit $ec } interfaceType="$1" if [ -z "$interfaceType" ]; then error_exit 1 "The interface type must be specified. (cli/gui)" elif [[ ! $interfaceType =~ ^(cli|gui)$ ]]; then error_exit 2 "The interface type must be 'cli' or 'gui'" fi echo $interfaceType
Source Link
cas
cas
  • 84.7k
  • 9
  • 138
  • 206

The main obvious problem with your script is a logic error. In your elif clause you are testing for ("not cli" OR "not gui") when you should be testing for ("not cli" AND "not GUI").

With the OR, the test fails if the variable doesn't match either "cli" or "gui". With the AND, it fails only when it doesn't match both.

For example:

if [ -z "$interfaceType" ]; then echo -e "[${c_RED}ERROR${c_RESET}] the interface type must be specified. (cli/gui)" && exit 1 elif [[ ! $interfaceType =~ ^[Cc][Ll][Ii]$ && ! $interfaceType =~ ^[Gg][Uu][Ii]$ ]]; then echo -e "[${c_RED}ERROR${c_RESET}] the interface type must be 'cli' or 'gui'" && exit 2 fi

However, that can be improved because regular expressions support alternations with the | pipe character. e.g.

if [ -z "$interfaceType" ]; then echo -e "[${c_RED}ERROR${c_RESET}] the interface type must be specified. (cli/gui)" && exit 1 elif [[ ! $interfaceType =~ ^([Cc][Ll][Ii]|[Gg][Uu][Ii])$ ]]; then echo -e "[${c_RED}ERROR${c_RESET}] the interface type must be 'cli' or 'gui'" && exit 2 fi

And can be further improved by setting nocasematch which enables case-insensitive matching for ==, != and =~ operators, as well as in case statements, pattern substitution (e.g. ${parameter/pattern/string}), and programmable completion.

From man bash:

nocasematch If set, bash matches patterns in a case-insensitive fashion when performing matching while executing case or [[ conditional commands, when performing pattern substitution word expansions, or when filtering possible completions as part of programmable completion.

Note that setting nocasematch persists until the end of the current script (or shell if you enter it on the command line). You may want to unset it after the regex test if you want case-sensitive matches for other tests.

shopt -s nocasematch if [ -z "$interfaceType" ]; then echo -e "[${c_RED}ERROR${c_RESET}] the interface type must be specified. (cli/gui)" && exit 1 elif [[ ! $interfaceType =~ ^(cli|gui)$ ]]; then echo -e "[${c_RED}ERROR${c_RESET}] the interface type must be 'cli' or 'gui'" && exit 2 fi

Finally, if you're going to check whether the variable matches neither cli or gui, then you don't also need to check if it's empty - because the empty string doesn't match either of those anyway. So, just the following would do:

shopt -s nocasematch if [[ ! $interfaceType =~ ^(cli|gui)$ ]]; then echo -e "[${c_RED}ERROR${c_RESET}] the interface type must be 'cli' or 'gui'" && exit 2 fi