trjhtr

I tend to dislike my code. It's always a mess. I can't critique my own code though, because I just turn it into another mess. I've been programming slightly better every year, but this year it has hardly changed.

#!/bin/bash

RED='e[101m'
NC='33[0m' # No Color

lesson[1]="#!/bin/bash"
lesson[2]="#!/bin/bash"

lessons=$#lesson[@]
current_lesson=1
speed=70

function start() 
 SECONDS=0 #restart timer

 reset_user_input
 string_to_type=$lesson[$1];
 length_of_string=$#string_to_type
 echo $string_to_type

 count=0
 while [ "$count" -ne "$length_of_string" ]; do
 checkCharacter
 done
 time_it_took=$SECONDS
 sec=60
 wpm=$((sec / time_it_took))
 wpm=$((wpm * length_of_string / 5))
 echo " "
 echo "speed: "$wpm


function checkCharacter() 
 IFS= read -r -s -n 1 user_input_char
 add_user_input $user_input_char

 if [ "$user_input_char" == " " ]; then
 add_user_input " "
 if [ "$user_input_char" == "$string_to_type:$count:1" ]; then
 printf " " 
 else
 printf $RED" "$NC
 fi
 else
 if [ "$user_input_char" == "$string_to_type:$count:1" ]; then
 printf $user_input_char
 else
 printf $RED$user_input_char$NC
 fi
 fi
 let count+=1


function check_correct() 
 if [ "$user_input" == "$string_to_type" ]; then
 return;
 else
 echo "too many mistakes, try again!"
 false;
 fi


function check_speed() 
 if [ "$wpm" -gt speed ]; then
 return;
 else
 echo "too slow, try again!"
 false
 fi



function start_lesson() 
 start $1
 if check_speed && check_correct; then
 echo "you succeeded!"
 echo " " # padding for every lesson
 return;
 fi
 echo " " # padding for every lesson
 false


function reset_user_input() 
 user_input=""


function add_user_input() 
 user_input=$user_input$1


while [ "$current_lesson" -le "$lessons" ]; do
 echo "lesson:" $current_lesson
 until start_lesson $current_lesson; do : ; done;
 let current_lesson+=1
done

I think this is very nice, clean code. Let's make it better!

Consistency

Consistency helps a lot, making the code easier to understand.

• Some functions are named camelCase, others snake_case. Stick to one pattern.


• Some variables are named SHOUT_CASE, others snake_case. Stick to one pattern. The latter is recommended, because upper case variable names are commonly used for system variables like PATH and others.


• The indentation is mostly 8 spaces, with few exceptions. Use the same everywhere.

Variable declaration

It's good to declare global constants at the top of the file where they are easy to see.
The same is not true for state variables that are implementation details,
such as current_lesson.
That variable would have been better to initialize right before you need it.

Iterating over arrays

Instead of a counting loop over the lesson array,
there is a more idiomatic approach:

for value in "$lesson[@]"; do ...; done

I would also rename the array to plural lessons.

The return value of functions

There are some subtle points I don't like in this code:

function check_correct() 
 if [ "$user_input" == "$string_to_type" ]; then
 return;
 else
 echo "too many mistakes, try again!"
 false;
 fi

A return command without parameter will use the exit code of the last statement. In this case it's success. I would find it clearer if you used an explicit return 0 there.

The else branch ends with a false statement.
If this is the last statement in the function,
then the exit code of the function will be 1, as intended.
But this is not explicit enough.
The function doesn't return immediately on the false statement.
An explicit return 1 would be much better.

Like this:

function check_correct() 
 if [ "$user_input" == "$string_to_type" ]; then
 return 0
 else
 echo "too many mistakes, try again!"
 return 1
 fi

With my other suggestions further down, I recommend to write this function like this:

check_correct() 
 if ! [ "$user_input" = "$string_to_type" ]; then
 echo "too many mistakes, try again!" >&2
 return 1
 fi

Undocumented features

The == operator is not documented in man [.
For that reason I recommend using = instead of == in [ ... ].

Style

The recommended function declaration style is without the function keyword:

start() {

There are a bunch of redundant semicolons at end of lines that can be safely removed.

Instead of this:

wpm=$((sec / time_it_took))

You can write slightly simpler like this:

((wpm = sec / time_it_took))

This works too:

((wpm *= length_of_string / 5))

I don't remember the explanation, but instead of let count+=1, ((count+=1)) is recommended. An even better way to write is ((count++)).

Sharpening the saw

I've been programming slightly better every year, but this year it has hardly changed.

Just keep doing it!

I type so fast your script had to stop with a division by zero error :D

line 64: [: speed: integer expression expected

This code segment

count=0
while [ "$count" -ne "$length_of_string" ]; do
 checkCharacter
done

is not transparent with how count is incremented. I would place (( count++ )) inside the loop instead of at the end of the checkCharacter function.

I see you're trying to attach meaningful names to these numbers, but you left out the magic number 5. I know it's the average number of English letters per word, but the number sticks out like a sore thumb among the other variable names.

time_it_took=$SECONDS
sec=60
wpm=$((sec / time_it_took))
wpm=$((wpm * length_of_string / 5))

I also dislike how the same name wpm is assigned different values in consecutive order. You could have just written wpm=$(( sec / time_it_took * length_of_string / 5 )) or given the first assignment a different name.

All in all, I would have written it differently and using fewer functions.