HiveBrain v1.2.0
Get Started
← Back to all entries
patternbashMinor

Find prime factors and reverse strings

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
factorsreversestringsfindandprime

Problem

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] [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 () {

Solution

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  [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.

Code Snippets

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

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

if ((count == 0)); then
    # ...
count=$count+1
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
is_prime "$j"
if [ $? != 1 ]; then

Context

StackExchange Code Review Q#154191, answer score: 5

Revisions (0)

No revisions yet.