Find prime factors and reverse strings

Question

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:"
    ""
    "1.     Help you reverse strings."
    "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
{
    telnet towel.blinkenlights.nl
}

#
# 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

Answer 1

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:

if is_prime "$j"; then

Performance

In is_prime, you loop from 2 until the target number. It would be enough to go until half of that.

Printing long texts

Your approach to print longer messages is clever with printf, but can be written simpler and easier using here-documents:

function usage_help
{
    cat << EOF
Use this script to:

1.     Help you reverse strings.
2.     Get the prime factors of numbers.
3.     Watch ASCII StarWars (ep IV).

USAGE:  [options] <command> [arguments]

...
EOF
}

Writing style

Although this is valid:

function version
{

The recommended modern writing style is this:

version() {

Redundant semicolons

Although not an error, the semicolons after the fi, done and return 0 are redundant:

for (( i=2; i<$1; i++ ))
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.

Answer 2

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.