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

Question

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

Answer 1

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.

Answer 2

Consider seq

Using seq makes your code significantly shorter in those for loops since you can just rely on seq to do your limit checking and the increments. beware that seq is non-standard though.

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

could simply be written as:

for i in $(seq 0 $i $limit) do

_{thanks to janos for pointing out the grievous flaws in the previous version of my answer}

To Space or not to space?

Compare your spacing in that for loop header:

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

stick to either always adding spaces around operators or not doing it at all. I recommend the former. This makes for a significant readability improvement, especially when not having syntax highlighting or similar.

I'm sure there's more, but that's about the whole extent of my bash knowledge, so I leave the rest in the hands of more competent reviewers :)

Answer 3

In addition to the loop improvements proposed in @Vogel612's answer you may check in the beginning of the script if the script arguments are numbers actually.

expr will give unexpected results or errors otherwise.

You can do so as proposed in this Q&A from Stack Overflow:

re='^[0-9]+$'
if ! [[ $1 =~ $re ]] || ! [[ $2 =~ $re ]]; then
    echo "error: Please provide 2 numeric parameters" >&2; exit 1
fi