Simple conditional yields too many responses

In this script:

#!/bin/bash
# bird
read -p "Enter name of a bird   "
REPLY=$REPLY
birdname="duck sparrow hawk"
for i in $birdname
do
        if [[ "$REPLY" = "$i" ]]
        then
                echo "Yes, that is a bird."
        else
                echo "That is not a bird."
        fi
done
 

I get this:

~/bin$ bird
Enter name of a bird   duck
Yes, that is a bird.
That is not a bird.
That is not a bird.
 

Ubuntu 18.04.2; Xfce 4.12.3; kernel 4.15.0-45-generic; bash 4.4.19(1); Dell Inspiron-518

If you find a match to your IF condition, you should exit otherwise you continue in the loop which explains why you have those lines, I let you figure out what to do the the other case, because you could have the case of nothing found and again you would have so many lines...
Hint: Use a variable you set and use it at the end once out of the loop...

1 Like

Your 'for' is going to create three data passes - one for each element in birdname.
Thus, correct in giving three results.
What were you hoping for, or expecting?

1 Like

Can I presume that if you enter hawk then the output you get is:-

~/bin$ bird
Enter name of a bird   hawk
That is not a bird.
That is not a bird.
Yes, that is a bird.

You code is doing what you are asking it to, but the issue (I presume) is a logical one.

You are checking if your input matches each of the items in turn, but you display the message every time. It would be better to define a flag/variable showing a failure before your loop then set the flag to a success within the loop if you meet the criteria. Then after the loop completes, you can display a message based on the flag.

Does that make sense? Sorry if it rather wordy.
Post your thoughts/next attempts and we can help with adjustments.

Kind regards,
Robin

1 Like

@joeyg--I wanted a single response, either "Yes, that is a bird." or "That is not a bird."
@vbe--I tried something else, but unfortunately it appears I need a stronger hint! Now, it works correctly for "duck", but does nothing for the other two birds, nor does it do anything for an incorrect response.

#!/bin/bash
# bird
read -rp "Enter name of a bird   "
REPLY=$REPLY
birdname="duck sparrow hawk"

for i in $birdname
do
     if [[ "$REPLY" = "$i" ]]; then
             echo "Yes, that is a bird."
     fi
     exit
     if [[ ! "$REPLY" = "$i" ]]; then
         echo "That is not a bird."
      fi
done

results:

:~/bin$ bird
Enter name of a bird   duck
 Yes, that is a bird.


:~/bin$ bird
 Enter name of a bird   sparrow
:~/bin$ 


:~/bin$ bird
  Enter name of a bird   hawk
:~/bin$ 


:~/bin$ bird
 Enter name of a bird    spaniel
:~/bin$

Then you want something closer to:

for i in $birdname
do
        if [[ "$REPLY" = "$i" ]]
        then
                Found = Found + 1
        else
                NotFound = NotFound +1
        fi
done

if $Found > 0
   then 
      echo "Yes, that is a bird."
   else
      echo "No, not a bird."
fi
1 Like
     if [[ "$REPLY" = "$i" ]]; then
             echo "Yes, that is a bird."
             exit
     fi
1 Like
birdnames="duck sparrow hawk"
read -p "Enter name of a bird   " name
if [[ " ${birdnames} " ==  *" ${name// } "* ]]; then
  echo "Yes, that is a bird."
else
  echo "That is not a bird."
fi

--
Or with a bash 4 associative array :

birdnames=(duck sparrow hawk "horned bill")
declare -A bird
for i in "${birdnames[@]}";
do
  bird[$i]=1
done

read -p "Enter name of a bird   " name
if [[ ${bird[$name]} ]]; then
  echo "Yes, that is a bird."
else
  echo "That is not a bird."
fi

or

birdnames=(duck sparrow hawk "horned bill")
declare -A bird
for i in "${birdnames[@]}";
do
  bird[$i]=
done

read -p "Enter name of a bird   " name
if [[ ${bird[$name]+1} ]]; then
  echo "Yes, that is a bird."
else
  echo "That is not a bird."
fi

or

declare -A bird
bird=([duck]= [sparrow]= [hawk]= [horned bill]=)

read -p "Enter name of a bird   " name
if [[ ${bird[$name]+1} ]]; then
  echo "Yes, that is a bird."
else
  echo "That is not a bird."
fi
1 Like

@rbattel--

Yes, that's exactly what I get. I will work on flag/variable success/failure suggestion.

@joeyg--I'm not sure how to use the Found/NotFound.

@vgersh99--I must have done something wrong. Couldn't get your suggestion to work.

@Scrutinizer--your first code worked exactly as I hoped for. Will get to array and declarations soon.

I suggest you start using real, telling variable names. You may not need it now for your example scripts but once you start writing real, working scripts you will have a hard time telling what all the i's, j's k's and what not are doing. If you search for a bird, call it "bird", not "i".

Furthermore, there is a reason why we almost religiously indent code. Consider the following code:

if <condition1> ; then
     command1
     if <condition2> ; then
          command2
     else
          command3
     fi
     command4
fi

When is "command1" executed? Answer: if "condition1" is met. When is "command3" executed? Answer: when "condition1" is met AND "condition2" is not met. And so on - the level of indentation tells you when (or: under which conditions) a certain part of the code is executed. In your code:

for i in $birdname
do
     if [[ "$REPLY" = "$i" ]]; then
             echo "Yes, that is a bird."
     fi
     exit
     if [[ ! "$REPLY" = "$i" ]]; then
         echo "That is not a bird."
      fi
done

When is the "exit" executed? Answer: every time. So, there is your answer why the program does not work the way you expected it. Ask yourself: under which circumstances do i want this command to be executed - and then you know where it is to be placed. We already established further up in the thread that the for-loop iterates through all the birds in your string. So ask yourself what you do want to do if the name is matched and what you want to do if not:

  • get first name
  • IF it matches, print "this is a bird" and exit
  • IF it doesn't match, then what?

Answer: do nothing. Return to the top and get the next name. Hence:

for bird in "$all_birds" ; do
     if [ "$bird" = "$REPLY" ] ; then
          echo "This is a bird"
          exit
     fi
done

This will say "This is a bird" in case it is - but it will say nothing if it isn't. Now, if something ever matches in this loop, the "exit" command is executed and the loop is aborted. When do we ever get to the end of the loop, hm? We get there only if nothing has matched, so where is the right place to put the negative "isn't a bird" message? Yes, right:

for bird in "$all_birds" ; do
     if [ "$bird" = "$REPLY" ] ; then
          echo "This is a bird"
          exit
     fi
done
echo "This is not a bird."

Put this into your program and try it.

@joeyg: Your solution is overly complicated and not completely logically sound. First, there is a variable "NOT_FOUND", which you increment but never use. What is the purpose of an unused variable.

Second: once you find a match you carry on till the end of the birds list. This only makes sense if the same bird is mentioned more than once. i.e.

all_birds="hawk dove hawk pidgeon hawk sparrow"
REPLY="hawk"

AND you want to find out how often it is mentioned in case it is mentioned at all.

If all the birds in the list are distinct this could never happen because every possible content of "$REPLY" can be matched by either one item in the list or by none at all. Therefore you do not need a flag because you can immediately act upon the condition instead of setting a flag and act on that later. You need flags only if you need to "store" the status of a condition and act upon it later. Like, if you want to connect several conditions which you cannot evaluate at once:

flag1=0
flag2=0
if <condition1> ; then
     flag1=1
fi
if <condition2> ; then
     flag2=1
fi

if (( flag1 + flag2 == 2 )) ; then        # only if both conditions were met (AND)
if (( flag1 + flag2 == 1 )) ; then         # only if exactly one condition was met (exclusive OR)
if (( flag1 + flag2 )) ; then             # if at least one condition was met (OR
if ! (( flag1 + flag2 )) ; then           # if neither of the conditions were met (NOT)

Or, if you need to string different actions on the conditions or their combination:

flag1=0
flag2=0
if <condition1> ; then
     flag1=1
fi
if <condition2> ; then
     flag2=1
fi

if (( flag1 + flag2 )) ; then
     if (( flag1 + flag2 == 2 )) ; then
          actionA
     fi
     actionB
else
     actionC
fi
if (( flag2 )) ; then
     actionD
fi

I hope this helps.

bakunin

1 Like