204

I tried to check if the PHONE_TYPE variable contains one of three valid values.

if [ "$PHONE_TYPE" != "NORTEL" ] || [ "$PHONE_TYPE" != "NEC" ] ||
   [ "$PHONE_TYPE" != "CISCO" ]
then
    echo "Phone type must be nortel,cisco or nec"
    exit
fi

The above code did not work for me, so I tried this instead:

if [ "$PHONE_TYPE" == "NORTEL" ] || [ "$PHONE_TYPE" == "NEC" ] ||
   [ "$PHONE_TYPE" == "CISCO" ]
then
    :        # do nothing
else
    echo "Phone type must be nortel,cisco or nec"
    exit
fi

Are there cleaner ways for this type of task?

ilkkachu
  • 138,973
munish
  • 7,987

8 Answers8

273

I guess you're looking for:

if [ "$PHONE_TYPE" != "NORTEL" ] && [ "$PHONE_TYPE" != "NEC" ] &&
   [ "$PHONE_TYPE" != "CISCO" ]

The rules for these equivalents are called De Morgan's laws and in your case meant:

not(A || B || C) => not(A) && not(B) && not (C)

Note the change in the boolean operator or and and.

Whereas you tried to do:

not(A || B || C) => not(A) || not(B) || not(C)

Which obviously doesn't work.

ilkkachu
  • 138,973
Nils Werner
  • 3,614
60

If using bash, a much shorter way would be:

if [[ ! $PHONE_TYPE =~ ^(NORTEL|NEC|CISCO)$ ]]; then 
  echo "Phone type must be nortel, cisco or nec."
fi
  • ^ – To match a starting at the beginning of line
  • $ – To match end of the line
  • =~ - Bash's built-in regular expression comparison operator
ANeves
  • 103
0x80
  • 893
  • 3
    The question doesn't mention bash, and in fact deliberately only mentions shell script, both in the tags, and in the question. This answer would fail in a POSIX environment. – Jack_Hu May 16 '21 at 17:21
  • If you're using Bash (recommended) as this answer, there's an even cleaner syntax you can use, please see my answer below. – Jesse Nickles Apr 23 '23 at 11:23
19

Good answers, and an invaluable lesson ;) Only want to supplement with a note.

What type of test one choose to use is highly dependent on code, structure, surroundings etc.

An alternative could be to use a switch or case statement as in:

case "$PHONE_TYPE" in
"NORTEL"|"NEC"|"CISCO")
    echo "OK"
    ;;
*)
    echo "Phone type must be nortel,cisco or nec"
    ;;
esac

As a second note you should be careful by using upper-case variable names. This is to prevent collision between variables introduced by the system, which almost always is all upper case. Thus $phone_type instead of $PHONE_TYPE.

Though that one is safe, if you have as habit using all upper case, one day you might say IFS="boo" and you're in a world of hurt.

It will also make it easier to spot which is what.

Not a have to but a would strongly consider.

It is also presumably a good candidate for a function. This mostly makes the code easier to read and maintain. E.g.:

valid_phone_type()
{
    case "$1" in
    "NORTEL"|"NEC")
        return 0;;
    *)
        echo "Model $1 is not supported"
        return 1;;
    esac
}

if ! valid_phone_type "$phone_type"; then
    echo "Bye."
    exit 1
fi
Runium
  • 28,811
14

You should use ANDs, not ORs.

if [ "$PHONE_TYPE" != "NORTEL" ] && [ "$PHONE_TYPE" != "NEC" ] && [ "$PHONE_TYPE" != "CISCO" ]
then

or 

if [ "$PHONE_TYPE" != "NORTEL" -a "$PHONE_TYPE" != "NEC" -a "$PHONE_TYPE" != "CISCO" ]
then
jlliagre
  • 61,204
2

Less portable for POSIX, but works in Bash:

if [[ $PHONE_TYPE != @(NORTEL|NEC|CISCO) ]]; then 
    echo 'Phone type must be NORTEL, CISCO, or NEC' >&2
    exit 1
fi
Kusalananda
  • 333,661
Jesse Nickles
  • 185
  • If anyone wants to know what the mods changed, my answer was if [[ "${PHONE_TYPE}" != @(NORTEL|NEC|CISCO)$ ]]; then which I still believe is much better syntax when using Bash... – Jesse Nickles Apr 25 '23 at 18:36
  • Jesse, the changes are visible in the edit history which can be seen by clicking the text "edited Apr 23 at 12:03" above the last editor's name. The editor left as title of the edit, in that edit history, the changes done: "Correct expression by removing traling $. Fix error message by mentionging the correct strings and outputting to standard error. Exit whith non-zero exit status to singnal error to caller. Remove unneeded quoting." – ANeves Nov 15 '23 at 11:52
  • In particular for your comment, they "removed unneeded quoting" (and the trailing dollar sign) — they changed from the original "${PHONE_TYPE}" directly to $PHONE_TYPE, as the variable is simple and does not need to be handled with the extra complication of "${FOO}" or even "$FOO", it can be used directly as $FOO. – ANeves Nov 15 '23 at 12:01
  • It is pretty much always safer to quote and { ... } variables in bash. My answer is valid and works, mods should not be changing answers because they merely have a different opinion and want to grow their meaningless karmaz... https://nickjanetakis.com/blog/why-you-should-put-braces-around-your-variables-when-shell-scripting – Jesse Nickles Nov 15 '23 at 15:26
1

To correct an above answer (as I can't comment yet):

PHONE_TYPE="NORTEL"
if [[ $PHONE_TYPE =~ ^(NORTEL|NEC|CISCO|SPACE TEL)$ ]]; then 
  echo "Phone type accepted."
else
  echo "Error! Phone type must be NORTEL, CISCO or NEC."
fi

Please note that you need at least bash 4 for this use of =~
It doesn't work in bash 3.

I tested on MS Windows 7 using bash 4.3.46 (works fine) and bash 3.1.17 (didn't work)

The LHS of the =~ should be in quotes. Above, PHONE_TYPE="SPACE TEL" would match too.

Will
  • 138
-1

Just a variation proposal based on @0x80 solution:

# define phone brand list
phoneBrandList=" NORTEL NEC CISCO" ## separator is space with an extra space in first place

# test if user given phone is contained in the list
if [[ ${phoneBrandList} =~ (^|[[:space:]])"${userPhoneBrand}"($|[[:space:]]) ]]; then
    echo "found it !"
fi
tdaget
  • 129
-2

Use [[ instead

if [[ "$PHONE_TYPE" != "NORTEL" ]] || [[ "$PHONE_TYPE" != "NEC" ]] || 
   [[ "$PHONE_TYPE" != "CISCO" ]]
then
echo "Phone type must be nortel,cisco or nec"
exit 1
fi
Swapnil
  • 121