Bash script to perform LUHN check

The following Bash script takes a single parameter (the PAN) and exits with 1 if the PAN does not satisfy a LUHN check.

#!/bin/bash

pan=$1
panlen=${#pan}

for i in $(seq $((panlen - 1)) -1 0); do
  digit=${pan:$i:1}
  if [ $(((panlen-i) % 2)) -eq 0 ]; then
     #even
     ((digit*=2))
     [ ${#digit} -eq 2 ] && digit=$((${digit:0:1}+${digit:1:1}))
  fi
  ((sum+=digit))
done

[ $((sum % 10)) -eq 0 ] || exit 1

For example, when placed in a file called isLUHNValid.sh it can be run with a valid PAN like:

./isLUHNValid.sh 4388576018410707 && echo 'Valid PAN' || echo 'Invalid PAN'

which will echo 'Valid PAN'.

And with an invalid PAN:

./isLUHNValid.sh 4388576018410708 && echo 'Valid PAN' || echo 'Invalid PAN'

it will echo 'Invalid PAN'.

I'm interested to know:

• if I've followed best practices; and/or

• if there's an opportunity for optimisation

I've been using Bash for a couple of years - but I'm constantly learning new things from examples on the Internet (like from here) and from man bash. Unfortunately in my position I don't have anyone to look over my shoulder and review by Bash code so I thought I'd take advantage of this community.

Unfortunately in my position I don't have anyone to look over my shoulder and review by Bash code so I thought I'd take advantage of this community.

My first reaction (as usual with any Bash review) was to copy-paste your code into shellcheck.net, and say that you do have someone to check your back, but your code passes with flying colors, so congrats! And yeah, we all have this issue, I hope that Code Review can make a difference for you.

It's a nice script and it runs well, but it can be better.

seq is not recommended, because it's not portable. It's not needed anymore anyway, because any modern Bash natively supports traditional for (( ; ; )) loops as in other languages, without having to spawn a sub-process. So the main loop can be written as:

for ((i = panlen - 1; i >= 0; i--)); do

Notice there I wrote spaces around operators. It's not an official recommendation, but I like this writing style, as recommended in other languages for increased readability. You can apply this style for all the code within ((...)), so for example instead of this:

((digit*=2))

I would write:

((digit *= 2))

Since you are already using ((...)) and as such the script is not POSIX compliant, I recommend all the way, and use it more aggressively for a simplified and more natural writing style.

For example instead of this:

if [ $(((panlen-i) % 2)) -eq 0 ]; then

You can write like this:

if (((panlen - i) % 2 == 0)); then

Comparisons with numbers are more natural in an arithmetic context within ((...)) than the -eq operator within [ ... ].

The final line of the program can be changed from this:

[ $((sum % 10)) -eq 0 ] || exit 1

To this:

((sum % 10 == 0))

That is, using a natural arithmetic context,
and taking advantage that the exit code of the program will be the exit code of the last command, making the || exit 1 redundant.
You may still want to leave the || exit 1 for clarity, it's up to you.

StackExchange Code Review Q#129649, answer score: 3