2
\$\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
asked Jan 30, 2018 at 12:55
\$\endgroup\$

1 Answer 1

3
\$\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$red2ドル 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
}
answered Jan 30, 2018 at 14:19
\$\endgroup\$
0

Your Answer

Draft saved
Draft discarded

Sign up or log in

Sign up using Google
Sign up using Email and Password

Post as a guest

Required, but never shown

Post as a guest

Required, but never shown

By clicking "Post Your Answer", you agree to our terms of service and acknowledge you have read our privacy policy.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.