Find prime factors and reverse strings

I'm currently taking an introductory course to Bash at my university, and was hoping to get some peer-review from whoever might have some time.

Full disclosure: the quality of the code isn't considered when the professor gives a grade as assignments only yield a PASS!

I've had very limited exposure to bash before taking this course, so I'm hoping You might have some understanding for any lackluster scripting.

Our current module covers functions, case-switches, loops and parameters (options, commands, arguments).

I just finished the last assignment and was hoping You could have a quick overview of it and see if I'm making any common mistakes and tell me how to correct them.

The same code may be easier to read on GitHub.

#!/bin/bash

# Script name
SCRIPT=$( basename "$0" )

# Script version
VERSION="1.0.0a"

#
# Usage / help message.
#

function usage_help
{
local txt=(
"Use this script to:"
""
"2.     Get the prime factors of numbers."
"3.     Watch ASCII StarWars (ep IV)."
""
"USAGE: $SCRIPT [options] <command> [arguments]" "" "OPTIONS:" " --help, -h Print help." " --version, -v Print version." "" "COMMAND: arguments in paranthesis ()" " reverse(string s) Reverses and prints string s" " factors(args) Prints the primefactors of submitted arguments." " starwars() Shows you an ASCII version of starwars over telnet." ) printf "%s\n" "${txt[@]}"
}

#
# Message to show when incorrect usage
#

function incorrect_usage
{
local message="$1" local txt=( "For an overview of the command, execute:" "$SCRIPT --help"
)

[[ $message ]] && printf "$message\n"

printf "%s\n" "${txt[@]}" } # # Message to display version # function version { local txt=( "$SCRIPT version $VERSION" ) printf "%s\n" "${txt[@]}"
}

#
# Helper function to check if number is prime
#

function is_prime () {
if [ "$1" == 1 ]; then return 1 fi; for (( i=2; i<$1; i++ ))
do
if (( $1%2 == 0)); then return 1 fi; done; return 0; } # # Main assignment, reverses submitted string - if possible. # function app-reverse { if [ "$1" != "" ];
then
echo "$1" | rev else echo "Empty argument, try again!" fi; } # # Main assignment, prints arguments' prime factors. # Uses is_prime as helper function. # function app-factors { re='^[0-9]+$' #numerical numbers
final_out=""
if [ "$1" == "" ]; then incorrect_usage "Please suply arguments! This counts as bad usage!" exit 1 else while [ "$1" != "" ];
do
out="$1: " if [[$1 =~ $re ]]; then count=0 for ((j=2; j<$1; j++))
do
is_prime "$j" if [$? != 1 ];
then
if (( $1%$j == 0));
then
out="$out$i"
count=$count+1 fi; fi; done if (($count == 0)); then
out="$out has no prime factorals." fi; else out="$out non-numerical parameter."
fi;
final_out="${final_out}"$'\n'"${out}" shift done echo "$final_out"
fi;
}

#
# Extra-assignment, for further credits.
#

function app-starwars
{
}

#
# Main script-body, iterates through submitted parameters
#

while (( $# )) do case "$1" in

--help | -h)
usage_help
exit 0
;;

--version | -v)
version
exit 0
;;

reverse     \
|   factors \
|   starwars)
command=$1 shift app-$command $* exit 0 ;; *) incorrect_usage "Options/command not recognized." exit 1 ;; esac done incorrect_usage exit 1  2 Answers Arithmetics in Bash In arithmetic contexts within ((...)) you can omit the $ in variable names (except the positional parameters $1, $2, ...),. For example instead of this:

if (( $1%$j == 0)); then
# ...

if (($count == 0)); then # ...  You can write slightly simpler as: if (($1 % j == 0)); then
# ...

if ((count == 0)); then
# ...


There is also a strange statement that looked like a bug at first:

count=$count+1  Given the initial value count=0, what will be the value of count after this statement? 0+1 That's not 1, but literally the string "0+1". This doesn't make the script broken, because the (($count == 0)) condition that uses this will still work as intended, but only by chance.

As it is, this statement is confusing and misleading. And you don't need a count, you need just a flag, to indicate that at least one factors were found, something like this:

found=
for ((j=2; j<$1; j++)); do is_prime "$j"
if [ $? != 1 ]; then if (($1 % $j == 0)); then out="$out $i" found=1 fi fi done if [ ! "$found" ]; then
out="$out has no prime factors." fi  Exit codes This is strange: is_prime "$j"
if [ $? != 1 ]; then  In a Bash shell, an exit code of 0 means success, non-0 means failure, as a general rule. The $? != 1 condition here seems to imply "success" (as in, "yes, the number is prime"). The straightforward way to convey that is to rewrite the condition using a value that actually means success:

is_prime "$j" if [$? = 0 ]; then


Use function exit codes directly

You can use the success/failure nature of exit codes directly in conditions. Instead of this:

is_prime "$j" if [$? != 1 ]; then


You can write in one statement as:

do
if (( $1%2 == 0)); then return 1 fi; done; return 0;  Formatting This may be a matter of taste, but I find this formatting strange: function is_prime () { if [ "$1" == 1 ];
then
return 1
fi;

for (( i=2; i<$1; i++ )) do if (($1%2 == 0));
then
return 1
fi;
done;
return 0;
}


Consider this alternative:

is_prime() {
if [ "$1" = 1 ]; then return 1 fi for ((i = 2; i <$1; i++)); do
if (($1 % 2 == 0)); then return 1 fi done }  Simplification Since non-empty values are truthy in Bash, instead of this: if [ "$1" != "" ];


You can write simpler like this:

if [ "\$1" ];


Early returns

When you return or exit in an if statement, you can eliminate the else statement, and make the rest of the function flatter, which is generally easier to read.

I wish I had more time to answer and go a bit more in-depth (and come up with sample code) but right away, it looks great! Superbly easy to read, especially for me as a non-frequent BASH user. Two possible suggestions:

1. Can you abstract the repeated requirement that the parameter cannot be empty? It's the first if statement in app-reverse and app-factors. It could either be something called, or as part of incorrect_usage.

2. That big nested else-while-if-for concerns me. I wonder if you can have the first statement be a guard statement to exit the function if the conditions aren't met. Then you don't need to nest everything under else:

if (!condition) {
return
}
// continue here...


Next, is there any way you could reduce or remove some of the loops? Referring to Why is using a shell loop to process text considered bad practice?, it may be considered best to avoid developing something hard to maintain. In my mind, a bash script is short and sweet. The loops make it more of an imperative program than quick script.

• I'll definetely have a look at those things! I'll hold off on marking this as the answer until I have some more answers, just in case! :) – geostocker Feb 1 '17 at 19:42