patternbashMinor
Find prime factors and reverse strings
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 () {
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
For example instead of this:
You can write slightly simpler as:
There is also a strange statement that looked like a bug at first:
Given the initial value
0+1
That's not 1, but literally the string "0+1".
This doesn't make the script broken,
because the
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:
Exit codes
This is strange:
In a Bash shell, an exit code of 0 means success, non-0 means failure, as a general rule.
The
The straightforward way to convey that is to rewrite the condition using a value that actually means success:
Use function exit codes directly
You can use the success/failure nature of exit codes directly in conditions.
Instead of this:
You can write in one statement as:
Performance
In
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
but can be written simpler and easier using here-documents:
Writing style
Although this is valid:
The recommended modern writing style is this:
Redundant semicolons
Although not an error, the semicolons after the
Formatting
This may be a matter of taste, but I find this formatting strange:
Consider this alternative:
Simplification
Since non-empty values are truthy in Bash, instead of this:
You can write simpler like this:
Early returns
When you return or exit in an
you can eliminate the
and make the rest of the function flatter,
which is generally easier to read.
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+1Given 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."
fiExit codes
This is strange:
is_prime "$j"
if [ $? != 1 ]; thenIn 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 ]; thenUse 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 ]; thenYou can write in one statement as:
if is_prime "$j"; thenPerformance
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+1found=
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."
fiis_prime "$j"
if [ $? != 1 ]; thenContext
StackExchange Code Review Q#154191, answer score: 5
Revisions (0)
No revisions yet.