Bash script to perform LUHN check

Question

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.

Answer 1

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.

Answer 2

With kudos to janos' excellent answer already I'd like to add:

wrap it in a function for reusability

While it looks silly for a trivial example, get in the habit of writing code inside of a function like so:

function foo {
        echo foo
}

foo

Later you can pull the function definition out into its own file and build your own library of reusable shell functions. Beyond reuse functions also provide optional variable locality:

function foo {
        local msg=boo
        echo $msg
}

msg=foo
foo
echo $msg

gives us output of

boo
foo

because the msg in the function is scoped to the function by local so it doesn't use or overwrite the global msg.

use env for portability

Having a shebang line like

#!/usr/bin/env bash

is more portable than the classic

#!/bin/bash

By use env we avoid requiring bash to be in /bin/. Maybe the user has compiled a newer bash and they want to try our script out with that. Isn't it easier to let the user alter the PATH before invoking our script rather than modifying it to point to their bash?

PATH=/home/chicks/newbash:$PATH test_script

The gnu docs show a number of other tricks you can perform with env.

performance possibility

I was too lazy to code and test this out, but since that seems to be a strong interest for you, I'll throw out: lookup tables. Rather than doing some of these calculations would it run faster if you had a big table of pre-calculated values? Calculating all of the answers ahead of time would result in a huge dataset that would be too big to be usable, but maybe calculating the sums for all of the 4 digit combinations and storing those in a hash would avoid the multiplications at runtime and squeeze out a few more nanoseconds.

use github

Using github or something similar is a good idea for backing up code and making it easy for other people to find it. Posting a link to your github in your question here makes it it possible for folks to follow the evolution of where you go with it beyond what is acceptable to post in your question here. [CR] does accept new questions with updated code, but you're not supposed to update the code in this question.

Answer 3

One thing I would change is to have the script call for the PAN number rather than having it passed to the script from the shell.

So rather than having:

pan=$1

I would use:

read pan

Or if you're comfortable with your typing skills, you can use read -s so it does not echo your PAN # on the screen while you type/paste it, like so:

read -s pan

What this change would do is eliminate the PAN # from being listed in plain text inside the history file.