1
\$\begingroup\$

My today's goal was to further practice POSIX shell scripting.

So, I decided to write the following POSIX shell script for generating pseudo-random numbers.

Any and all reviews would be appreciated.

#!/bin/sh

bold=$(tput bold)
red=$(tput setaf 1)
nocolor=$(tput sgr0)

bold_red="${bold}${red}"

print_error_and_exit()
{
    # check if exactly two arguments have been passed
    if [ "$#" -ne 2 ]
    then
        print_error_and_exit "print_error_and_exit(): There have not been passed two arguments!" 2
    fi

    # check if the second argument is a number
    if ! is_this_a_number "$2"
    then
        print_error_and_exit "print_error_and_exit(): The argument #2 is not a number!" 5
    fi

    echo "${bold_red}$1 Exit code = $2.${nocolor}" 1>&2
    exit "$2"
}

is_this_a_number()
{
    # check if exactly one argument has been passed
    if [ "$#" -ne 1 ]
    then
        print_error_and_exit "is_this_a_number(): There has not been passed exactly one argument!" 2
    fi

    # check if the argument is an integer number
    if [ "$1" -eq "$1" ] 2> /dev/null
    then
        return 0
    else
        return 1
    fi
}

random_number()
{
    # check if exactly two arguments have been passed
    if [ "$#" -ne 2 ]
    then
        print_error_and_exit "random_number(): There have not been passed exactly two arguments!" 3
    fi

    # check if the first argument is a number
    if ! is_this_a_number "$1"
    then
        print_error_and_exit "random_number(): The argument #1 is not a number!" 4
    fi

    # check if the second argument is a number
    if ! is_this_a_number "$2"
    then
        print_error_and_exit "random_number(): The argument #2 is not a number!" 5
    fi

    # min is inclusive
    range_min="$1"

    # max is inclusive too
    range_max="$2"

    # RANDOM is not POSIX-available,
    # so we can use shuf for instance
    shuf -i "$range_min"-"$range_max" -n 1
}


# temporary testing loop
i=1
while [ "$i" -lt 10 ]
do
    random_number 1 9

    i=$((i + 1))
done
\$\endgroup\$

1 Answer 1

1
\$\begingroup\$

Firstly, it's good that you use tput to portably obtain terminal codes if they exist. Too many shell script authors fail to do that! We might choose to only evaluate those if we know we'll use them (i.e. inside print_error_and_exit), and there seems little point in setting $bold_red to use only once.

I wonder if the error checking within print_error_and_exit might be overkill. If you're using it only within this script, you might omit the argument checking (if you made a mistake in the recursive call, you might end up in a worse position than not checking at all!).

It might be worth accepting the arguments to print_error_and_exit in the opposite order (exit code first), to avoid the important part being overlooked at the end of the line.

Some of the if tests are very verbose compared to simply chaining commands with && or ||. That might be a style decision, but I would certainly reconsider

if command
then
    return 0
else
    return 1
fi

When it's the last line of the function, that simplifies to just

command

(unless you really care which non-zero value indicates failure).

Although we know that $# expands to an integer, and is therefore safe to use without quoting, there's nothing wrong with quoting it as you have (and here on Stack Exchange it helps the prettifier distinguish it from a comment).

I was concerned that shuf might be very inefficient if a large range is selected, but reading the source for the GNU coreutils implementation shows that it manages to avoid building the entire list of numbers to choose from. Whether other implementations are similarly optimised is left as an exercise.

Modified program

#!/bin/sh

print_error_and_exit()
{
    # check if exactly two arguments have been passed
    test "$#" = 2 || print_error_and_exit 2 "print_error_and_exit(): There have not been passed two arguments!"

    # check that the first argument is a number
    is_this_a_number "$1" || print_error_and_exit 5 "print_error_and_exit(): The argument #1 is not a number!"

    bold=$(tput bold)
    red=$(tput setaf 1)
    nocolor=$(tput sgr0)

    echo "$bold$red$2 Exit code = $1.$nocolor" >&2
    exit $1
}

is_this_a_number()
{
    # check if exactly one argument has been passed
    test "$#" = 1 || print_error_and_exit 2 "is_this_a_number(): There has not been passed exactly one argument!"
    # check if the argument is an integer
    test "$1" -eq "$1" 2>/dev/null
}

# print a number in range $1 to $2, inclusive
random_number()
{
    # check if exactly two arguments have been passed
    test "$#" = 2 || print_error_and_exit 3 "random_number(): There have not been passed exactly two arguments!"

    # check if the arguments are both numbers
    is_this_a_number "$1" || print_error_and_exit 4 "random_number(): The argument #1 is not a number!"
    is_this_a_number "$2" || print_error_and_exit 5 "random_number(): The argument #2 is not a number!"

    shuf -i "$1-$2" -n 1
}
\$\endgroup\$
0

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

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