2
\$\begingroup\$

Rules

  • 7 mistakes allowed.
  • Hangman rules.

Disclaimer

I don't have a figure (drawing) of the hangman as this project took way longer than I thought I would take. It took like 9 hours during 3 weeks.

General mistakes (I could detect but can't program away):

  1. Poor way of generating the random word to be guessed.

  2. I have dummy code that isn't used ( I don't know how to remove this without going line for line )

  3. I'm used to MVC frameworks but my display functions seem to need some calculating code, because things get difficult when I keep them in a separate function.

Reflection

I am doing more of these little assignments and realized my "good code" and "fast/dirty code" both are poor and don't differ that much.

Where does one learn how to categorize your code

Does anyone know a book that like explains which kind of code needs to be in which kind of function? As I feel I'm reinventing the wheel. Like how do you guys feel that code should be in another place?

#!/bin/bash

words[1]="twitter"
words[2]="facebook"
words[3]="myspace"
words[4]="youtube"
words[5]="amazon"
random_nr=6
correct_letters[1]=""
mistake_letters[1]=""
imploded_letters[1]=""
geuss_counter=1
correct_counter=0
mistakes=0
place=1

# setters
set_word() {
    until [[ $random_nr -lt 6 ]] && [[ $random_nr -gt 0 ]]; do 
        random_nr=${RANDOM};
        word=${words[$random_nr]}
    done
}

set_word_length() {
    word_length_incl_zero=${#word}
    word_length=$((word_length_incl_zero+1))
}

# views
display_result() {
    for i in $(seq 1 $word_length); do
        if [[ $i -ne $word_length ]]; then
            position=$((i-1))
            if [[ ${word:position:1} == $user_input ]]; then
                correct_counter=$((correct_counter+1))
                correct_letters[$correct_counter]=$user_input

                continue_game
            fi
        fi
    done

    echo "sorry you're wrong"

    mistakes=$((mistakes+1))
    mistake_letters[$mistakes]=$user_input
}

display_welcome_message() {
    echo "GALGJE"
    echo
    echo "Hello, so happy to see you playing my handmade game"
    echo "Basicly you need to guess every letter of the word below"
    echo "Before things go badly with the guy on the left"
    echo
}

display_dashes() {
    letters_not_guessed=0
    for i in $(seq 1 $word_length); do
        if [[ $i -ne $word_length ]]; then
            position=$((i-1))
            check_if_already_guessed
            if [[ $found -eq 1 ]]; then
                echo -n ${word:position:1}
            else
                letters_not_guessed=$((letters_not_guessed+1))
                echo -n "-"
            fi
        fi
    done
    echo
    echo
}

check_if_already_guessed() {
    found=0
    for i in "${correct_letters[@]}"
    do
        if [ "$i" == "${word:position:1}" ] ; then
            found=1
        fi
    done
}

display_asking_for_a_letter() {
    if [[ $letters_not_guessed == 0 ]]; then
        echo "you've won"
        exit
    fi
    read -sn 1 user_input
    echo
}

display_mistakes() {
    if [[ $mistakes -lt 7 ]]; then
        for i in $(seq 1 ${#mistake_letters[@]}); do
            if [[ $mistakes -eq 1 ]]; then
                more_mistakes_than_one=""
            else
                more_mistakes_than_one="s"
            fi

            if [[ $i -eq 1 ]]; then
                echo -n "wrong ($mistakes) letter$more_mistakes_than_one: "
                echo -n ${mistake_letters[$i]}
            else
                echo -n ", ${mistake_letters[$i]}"
            fi
        done
        echo
    else
        end_game
    fi
}

display_correct_letters() {
    if [[ $mistakes -lt 7 ]] && [[ $correct_counter -lt $word_length ]]; then
        for i in $(seq 1 ${#correct_letters[@]}); do
            if [[ $correct_counter -eq 1 ]]; then
                more_correct_than_one=""
            else
                more_correct__than_one="s"
            fi

            if [[ $i -eq 1 ]]; then
                echo -n "right ($correct_counter) letter$more_correct_than_one: "
                echo -n ${correct_letters[$i]}
            else
                echo -n ", ${correct_letters[$i]}"
            fi
        done
        echo
        echo
    else
        end_game
    fi
}

continue_game() {
    display_mistakes
    display_correct_letters
    geuss_counter=$((geuss_counter+1))
    main
}

end_game() {
    echo "sorry you've lost"
    exit
}

get_first_time_guess() {
    first_time_guess=1

    for letter in "${correct_letters[@]}"
    do
        if [ "$letter" == "$user_input" ] ; then
            first_time_guess=0
        fi
    done

    for letter in "${mistake_letters[@]}"
    do
        if [ "$letter" == "$user_input" ] ; then
            first_time_guess=0
        fi
    done
}

main() {
    #visuals
    display_dashes
    display_asking_for_a_letter

    #if this letter is already guessed the user
    #shouldnt be penalized for it
    get_first_time_guess
    if [[ $first_time_guess -eq 1 ]]; then
        display_result
    fi

    continue_game
}

set_word
set_word_length
display_welcome_message
main
\$\endgroup\$
3
\$\begingroup\$

Code organization and naming

It's good that you organized your code into functions, and that you set everything in motion after all the function definitions, using a few simple terms that drive your program:

set_word
set_word_length
display_welcome_message
main

It's a pity that the simple terms that drive the program are not human-friendly, and not natural. Better names and some rethinking can help:

  • set_word -> My first reaction reading this is "what word?", and "set to what?". If you play this game with a friend, would you ask him to "set word"? Probably you'd phrase it more like "pick a random word". In a program, the name init_random_word would sound quite natural to me.

  • set_word_length -> Again, my first reaction is "set to what?" More importantly, the purpose of this unclear, I have to read the implementation to understand. The fact that you need a word_length variable is a low-level implementation detail, and as such it would be better to not require this function call at the top-level. It would be better to move this step inside the init_random_word function, directly (the code itself) or indirectly (call to the function that performs this step).

  • display_welcome_message -> This is good as it is, perfectly clear what it will do and why.

  • main -> A more natural and intuitive name would be run_hangman_game. This and the other calls could actually be in a function called main, and you could call main to setup and run the game.

As to your specific question about code organization:

Does anyone know a book that like explains which kind of code needs to be in which kind of function? [...] Like how do you guys feel that code should be in another place?

I recommend the book Code Complete. It's not a direct answer to your question, but it's my favorite fundamental book for programmers (especially the chapter on Abstract Data Types), and I think it will guide you on the path of developing good programmer instincts. As a more direct answer, I suggest to read up on SOLID principles, especially the Single Responsibility Principle is relevant for designing reusable, testable program elements.

Picking a random word

This is an extremely poor implementation of picking a random word from an array:

until [[ $random_nr -lt 6 ]] && [[ $random_nr -gt 0 ]]; do 
    random_nr=${RANDOM};
    word=${words[$random_nr]}
done

One big problem is that RANDOM returns a random value between 0 and 32767. It can be a lot of wasted cycles until you get a number between 1 and 5. The common approach is to get a number between 0 and n using modulo:

num=$((RANDOM % n))

The same thing, but in a slightly more natural and readable form in Bash:

((num = RANDOM % n))

Be careful, because this formula gets a random number between 0 and n, not 1 and n. The current program needs a random number between 1 and 5, because words[0] is not assigned, only words[1] ... words[5]. You could adjust the formula to account for that:

((num = 1 + RANDOM % (n - 1)))

But a much simpler way will be to change the program, to require a number in the range of 0 and n, by assigning values in words starting from element 0 instead of 1. This is also the most natural way to work with arrays with 0-based indexes as in Bash, and in most programming languages.

Note that we can assign all elements at once, which is shorter, and most importantly, we don't need to worry about writing the correct indexes:

words=("twitter" "facebook" "myspace" "youtube" "amazon")

And then we can get a random word from this array with:

word=${words[RANDOM % ${#words[@]}]}

This is safe, fast, and simple. We can also eliminate the random_nr variable and the set_word_length function, they are simply no longer needed.

Arithmetic context in Bash

As mentioned earlier, instead of this:

mistakes=$((mistakes+1))

This is a simpler syntax:

((mistakes = mistakes + 1))

Notice that within the arithmetic context ((...)) the $ is optional in variable names (except some special variables such as the positional variables $1, $2, ... and a few others).

And this example can be further simplified to either of these forms:

((mistakes += 1))
((mistakes++))

Iterating over letters of a string

Avoid seq as much as possible, it's not available in many environments, and a native Bash alternative exists, in the form of counting loops. For example, instead of this:

for i in $(seq 1 $word_length); do

You can write in native Bash:

for ((i = 1; i <= word_length; i++)); do

(Notice that I didn't write $ in front of i and word_length, since the expressions are within an arithmetic context ((...)), the $ are unnecessary.)

Now let's fix the loop in the code:

for i in $(seq 1 $word_length); do
    if [[ $i -ne $word_length ]]; then
        position=$((i-1))
        if [[ ${word:position:1} == $user_input ]]; then
            correct_counter=$((correct_counter+1))
            correct_letters[$correct_counter]=$user_input

            continue_game
        fi
    fi
done

Not only seq is inferior to native Bash counting loops, it also forced you to add a conditional $i -ne $word_length in the loop body, and then you also needed to compute position as $((i-1)). All this complexity can go away if you rewrite with a counting loop:

for ((i = 0; i < ${#word}; i++)); do
    if [[ ${word:i:1} == $user_input ]]; then
        ((correct_counter++))
        correct_letters[$correct_counter]=$user_input

        continue_game
    fi
done

Notice that I didn't use the word_length variable, it's no longer necessary here. I suggest to apply the same technique to eliminate it from the rest of the program.

Code organization 2

Now that I've reached the implementation of the display_result function, I see that it calls a continue_game function from its loop, which will call main (among other functions). These recursive calls generate a deep stack, which is difficult to understand and follow.

A better way is to reorganize the program without such recursive calls. Use a loop that takes user input, checks the result, tracks the program state, and either continues to take user input or terminates with the final result (user won or lost).

Minor things

  • The program is full of typos and grammar mistakes. It's good to pay attention to details.

  • imploded_letters and place are defined but not used. Delete them.

  • correct_counter and mistakes should not be necessary. With minor changes, you can get these values from ${#correct_letters[@]} and ${#mistake_letters[@]}, respectively. Note also that it would be better to initialize these arrays as correct_letters=() and mistake_letters=(), and to append values to them with correct_letters+=($user_input).

  • The flags of echo are not portable, such as -n. Without flags, echo is fine. If you really want to suppress the newline character, a possible alternative is printf, if you don't mind that it's not POSIX. If possible, it's best to find a way to avoid both, for example by designing the user interface in such a way that it doesn't need to suppress the newline anywhere.

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