2
\$\begingroup\$

I use this bash script to review over vocabulary with my dental Spanish class. The script works great for me. However, I am wondering what other ways the same thing could be done. How could it be more efficient? Perhaps the script could me made more readable? Maybe other areas could be made more proper or elegant? If I have done something well, then that feedback too is appreciated. I am not particularly interested in adding in checks/tests of the formatting of my vocabulary sheet or checking to make sure the file exists. The comments have my English translations of the Spanish in the script.

This script reads each line of the vocabulary file from the top to the bottom and tests whether the user knows how to say the equivalent in Spanish. It is formatted like this, English: SpanishOpt1, SpanishOpt2, SpanishOpt3. Any of the options between commas could count as correct.

This is a sample of what a vocab file looks like:

floor of the mouth: piso de la boca
cheek: mejilla, cachete
throat: garganta
change in color: cambio en color, cambio de color
taste buds: bulbos gustativos
growth: crecimiento
bruise: moretón, cardenal, magulladura, moradura, contusión

Here is the script:

#!/bin/bash

read -r -p "1 - evaluación oral
2 - radiografía
Elige uno: " opt
# Choose one.

clear

if ! [[ "$opt" =~ ^[1-2]$ ]]; then
    # That is not an option. Choose 1 or 2. 
    echo "Eso no es una opción. Elige 1 o 2."
    exit 1
fi

fname=("" "evaluación-oral-vocabulario.txt" "radiografía-vocabulario.txt")
file="/home/brock/2-Work/Dental/${fname[$opt]}"
vocab_items=$(wc -l < "$file")
line_num=1
num_right=0

until (("$line_num" > "$vocab_items")); do
    vocab_item=$(head -n"$line_num" "$file" | tail -n1)
    english=$(echo "$vocab_item" | cut -d: -f1)
    spanish=$(echo "$vocab_item" | cut -d: -f2 | sed 's/ //')
    plural_options=$(echo "$spanish" | grep ,)
    plural_vocab_item=$(echo "$spanish" | cut -d, -f1 | wc -w) 
    if [ -n "$plural_options" ] || (("$plural_vocab_item" > "1")); then
        # The "s" is to make the subject below "Las palabras" plural if needed. The "n" is for the plural verb conjugation.
        s="s"; n="n"
    else    
        s=""; n=""
    fi
    # How do you say ... in Spanish?
    read -r -p "¿Cómo se dice \"$english\" en español? " r
    answer_check=$(echo "_${spanish}_" | sed 's/, /_/g' | grep "_${r}_")
    # The "y" means "and" which is added before the last item in a list with no serial comma.
    spanish_feedback=$(echo "$spanish" | sed 's/\(.*\),/\1 y/')
    if [ -n "$answer_check" ]; then
        # You are correct. The word(s) ... mean ...
        echo -e "\tTienes razón. La$s palabra$s \"$spanish_feedback\" quiere$n decir \"$english\".\n"
        ((num_right++)) 
    else    
        # You are not correct. The word(s) ... mean ...
        echo -e "\tNo tienes razón. La$s palabra$s \"$spanish_feedback\" quiere$n decir \"$english\".\n"
        pair_wrong[$line_num]="$vocab_item\n"
    fi  
    ((line_num++))
done

# You know ... out of ... terms overall.
echo "Tú sabes $num_right de $vocab_items términos en total."
if [ -z "${pair_wrong[*]}" ]; then
    # Incredible! One hundred percent!
    echo "¡Increíble! ¡Cien por ciento!"
else
    # You have to work more on:
    echo -e "Tienes que trabajar más en:\n ${pair_wrong[*]}"
fi
\$\endgroup\$
  • \$\begingroup\$ Please do not edit your code to implement the suggestions. If you would like your modified code to be reviewed, you must ask a new question. \$\endgroup\$ – Donald.McLean May 8 '17 at 19:27
  • \$\begingroup\$ Why is that an issue? Thanks \$\endgroup\$ – jbrock May 8 '17 at 19:37
  • \$\begingroup\$ @jbrock doing so goes against the Question + Answer style of Code Review. This is not a forum where you should keep the most updated version in your question. Please see what you may and may not do after receiving answers. \$\endgroup\$ – Simon Forsberg May 8 '17 at 20:11
  • \$\begingroup\$ @SimonForsberg I appreciate the clarification. \$\endgroup\$ – jbrock May 8 '17 at 20:16
  • \$\begingroup\$ Whilst updating with the answer is against the style and rules, posting a new question where you include suggestions from the given answers is allowed. \$\endgroup\$ – holroy May 8 '17 at 20:19
2
\$\begingroup\$

Validating discrete choices

This is fine:

if ! [[ "$opt" =~ ^[1-2]$ ]]; then
    # That is not an option. Choose 1 or 2. 
    echo "Eso no es una opción. Elige 1 o 2."
    exit 1
fi

Another way could be using a case statement:

case "$opt" in
    [12]) ;;
    *) 
    # That is not an option. Choose 1 or 2. 
    echo "Eso no es una opción. Elige 1 o 2."
    exit 1
esac

The advantage of this is that case uses simple glob patterns, which may be easier to get right than the regex syntax in a [[ ... ]].

This alternative gets even better if we combine it with the file selection:

basedir="/home/brock/2-Work/Dental"

case "$opt" in
    1) file=$basedir/evaluación-oral-vocabulario.txt ;;
    2) file=$basedir/radiografía-vocabulario.txt ;;
    *) 
    # That is not an option. Choose 1 or 2. 
    echo "Eso no es una opción. Elige 1 o 2."
    exit 1
esac

Variables in arithmetic context

Instead of this:

until (("$line_num" > "$vocab_items")); do

You don't need to quote numeric variables, and you don't even need the dollars, this works:

until ((line_num > vocab_items)); do  

Picking a specific line from a file

Instead of this:

vocab_item=$(head -n"$line_num" "$file" | tail -n1)

This is more efficient:

vocab_item=$(sed -ne ${line_num}p)

Splitting a line into an array

@holroy already pointed out how to split the line by : to get the English and Spanish parts. You could use the same technique to split the Spanish by commas. That will also spare you the wc -w call to get the number of items, as you will be able to get it directly from the Bash array.

Emptying variables

Instead of this:

s=""; n=""

A simpler way to empty variables:

s=
n=

You could write s=; n= but it's not recommended to write multiple statements on one line.

Pattern substitution

Instead of a pipeline of echo and sed, you could use the built-in pattern substitution features of Bash. I would rewrite this part:

answer_check=$(echo "_${spanish}_" | sed 's/, /_/g' | grep "_${r}_")
spanish_feedback=$(echo "$spanish" | sed 's/\(.*\),/\1 y/')
if [ -n "$answer_check" ]; then
    # ...

Like this, using native Bash features:

spanish_feedback=${spanish//,/ y/}
if [[ _${spanish//, /_}_ == *_${r}_* ]]; then
    # ...
\$\endgroup\$
  • \$\begingroup\$ You are bringing back memories of various tips and tricks of Bash. Good reading... :-) \$\endgroup\$ – holroy May 5 '17 at 21:58
  • \$\begingroup\$ @janos I had to stick with the wc -w because it checks to see if more than one word was between the commas on the Spanish answer options. It also did not seem fitting to split the Spanish part into an array because it seems I would then need to loop through each item to see if it matches what the user input. If one item matched then count as correct and leave the loop. If none match then it would be counted wrong. So, that sounded a bit more complicated than using grep. \$\endgroup\$ – jbrock May 8 '17 at 0:44
  • \$\begingroup\$ @janos I was unable to do a re-write for your last section titled Pattern substitution. I needed to grep for the user's answer input and then check to see if it was one of the items in the list of possible correct answers for example hat could be gorra, gorro, or sombrero. I also had already tried the builtin Bash pattern substition, but I needed to use sed because only the last comma is replaced by a ' y'. Other than this your answer has some really helpful tips. Thank you. \$\endgroup\$ – jbrock May 8 '17 at 0:45
  • \$\begingroup\$ @jbrock I don't understand your points. I think you probably made some mistakes​ in your attempts. I will write up a more complete example in my lunch break or in the evening to show you \$\endgroup\$ – janos May 8 '17 at 6:20
  • \$\begingroup\$ @janos I have updated my script (in question above) to include the last part on your answer "Pattern substitution" (everything but the feedback variable). I don't know why I thought the if statement wouldn't work. It is nice and succinct though. \$\endgroup\$ – jbrock May 8 '17 at 19:25
2
\$\begingroup\$

I haven't used or reviewed bash script for ever, so I'm just going to give a few comments:

  • Your script reads nicely, and even though I don't know spanish, there are some comments explaining the vital parts.
  • I think the script could benefit from a little more spacing, so that the various sections stood out a little more. The top half is better than the bottom half.
  • The variable names are good and helpful
  • However there are two code smells that I see:
    • Each time you set vocab_item you're re-reading the file. So at end of script you've read the file \$N\$ times, where \$N\$ is the number of lines
    • Each time you set english and spanish to trigger a new call to cut.

Reading the main file once

After a little searching I found this thread on SO:Looping through the content of a file in bash. The accepted answers uses the following construct:

while read p; do
  echo $p
done <peptides.txt

This would allow for reading the file just once, but you'll need to change a little of your logic related to counting the correct number of answers.

Splitting a line into an array

Another search, and another SO thread: Split string into an array in bash. This one to avoid the calls to cut.

IFS=', ' read -r -a parts <<< "$vocab_item"
english="${parts[0]}"
spanish="${parts[1]}"

Hope this helps to some extent.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.