4
\$\begingroup\$

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='\033[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
\$\endgroup\$
2
\$\begingroup\$

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!

\$\endgroup\$
1
\$\begingroup\$

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.

\$\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.