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

Shell Script (Bash) to sum multiples of different arguments below a number

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

Problem

I'm interested in learning how to create shell scripts with bash so here's one of my first exercises, taken from here. It is an extension of the problems Elementary - 4 and 5.

The goal of the program is to sum all multiples or ARG1 or ARG2 below the user's input number.

#!/bin/bash
echo "This program will find all the multiples of ARG1 or ARG2 below or equal to your number and sum them up. Please input your limit:"
read limit

commonMultiple=`expr $1 \* $2`

sumArg1=0
sumArg2=0
sumCommonMultiple=0

for (( i = 0; i <= $limit; i+=$1 )); do
    sumArg1=`expr $sumArg1 + $i`
done
for (( i = 0; i <= $limit; i+=$2 )); do
    sumArg2=`expr $sumArg2 + $i`
done
for (( i = 0; i <= $limit; i+=$commonMultiple )); do
    sumCommonMultiple=`expr $sumCommonMultiple + $i`
done

echo "The result is `expr $sumArg1 + $sumArg2 - $sumCommonMultiple`."


This works perfectly but I'd like the input of more experienced programmers. I don't think my logic is efficient. There must be something obvious I'm missing that could have made my code a lot simpler.

Disclaimer: apparently some of the stuff I used is outdated. I'll take it into account for my future code.*

Solution

Arithmetic operations

If you look closely, something is a bit odd here:

for (( i = 0; i <= $limit; i+=$1 )); do


Ideas? How about: i is written without a dollar, but limit is written with a dollar... In fact you can drop the dollar from limit too, and the expression will be slightly simpler. From $1 you cannot drop it, as the meaning of 1 would be taken literally, because it's numeric, unlike the others.

The same is true for commonMultiple here, you can drop the dollar:

for (( i = 0; i <= $limit; i+=$commonMultiple )); do


The general rule is, in arithmetic operations within ((...)) you don't need to use the dollars in variable names.

Don't use backticks

Instead of this:

commonMultiple=`expr $1 \* $2`


This is the recommended writing style for sub shells:

commonMultiple=$(expr $1 \* $2)


Don't use expr

It's better to avoid expr, and you can do the same thing easier using Bash arithmetics, for example:

((commonMultiple = $1 * $2))


The for loops can be simplified similarly, instead of:

for (( i = 0; i <= $limit; i+=$1 )); do
    sumArg1=`expr $sumArg1 + $i`
done


You can write:

for (( i = 0; i <= limit; i+=$1 )); do
    ((sumArg1 += i))
done


Apply the same thing everywhere.

Code Snippets

for (( i = 0; i <= $limit; i+=$1 )); do
for (( i = 0; i <= $limit; i+=$commonMultiple )); do
commonMultiple=`expr $1 \* $2`
commonMultiple=$(expr $1 \* $2)
((commonMultiple = $1 * $2))

Context

StackExchange Code Review Q#112806, answer score: 10

Revisions (0)

No revisions yet.