5
\$\begingroup\$

I implemented Luhn's algorithm explained on Wikipedia to validate a credit card number, in Bash.

Does this code follow good practices?

#!/bin/bash
getchecknumber(){ # get the last digit of the cc number
    str=$1
    checknum=$(echo ${str: -1})
}
getrest(){ # get the rest of the numbers in the sequence
    str=$(echo ${str%?})
}
doubleeveryother(){ # double every other number
    # split characters into an array
    arr=()
    i=0
    while [ "$i" -lt "${#str}" ]; do
        arr+=(${str:$i:1})
        i=$((i+1))
    done
    for ((i=1; i<=15; i++))
    do  {
        char=$(echo "$i-1" | bc)
        double=$(echo "${arr[$char]} * 2" | bc)
        if [ $(($i%2)) -eq 0 ] ; then foo=bar; else arr[$char]=$double #replace the number in the array
        fi
    }
    done
    echo -e
}
adddigitsum() { #split a two digit number into two numbers and add them
    for ((a=0; a<=14; a++))
    do {

        NUMSPLIT=()
        number=$(echo ${arr[$a]})
        if [[ ${#number} == 2 ]] ; then
            i=0
            while [[ "$i" -lt ${#number} ]]; do
                NUMSPLIT+=(${number:$i:1})
                i=$((i+1))
                done
            num=$((${NUMSPLIT[0]} + ${NUMSPLIT[1]}))
            addeddigit=$( echo $num | bc )
            arr[$a]=$addeddigit # replace the number in the array
        fi
    }
    done
}
addall(){ #add all the digits
    sum=$( IFS="+"; bc <<< "${arr[*]}" )
}
multiply(){ #multiply by 9
    result=$( echo "$sum * 9" | bc )
}
validate(){ #see if the last number of 'result' is equal to the checknum
    resultchecknum=$(echo ${result: -1})
    if [[ $resultchecknum == $checknum ]] ; then echo "CC Number is valid!" ; else echo "CC Number is not valid."
    fi
}
echo "$1 "
getchecknumber $1
getrest
doubleeveryother
adddigitsum
addall
multiply
validate
\$\endgroup\$
4
\$\begingroup\$
if [ $(($i%2)) -eq 0 ] ; then foo=bar; else arr[$char]=$double

Then foo=bar? In any case, an alternative way of writing this is something like (borrowing your final if statement):

[[ $resultchecknum == $checknum ]] && echo "CC Number is valid!" \
                                    || echo "CC Number is not valid."

Also, you have a mix of echo "..." | bc and bc <<< "...", I suggest keeping to the latter as it simpler to write.

A similar alternative to if-then-else is using &&:

[ ! $(($i%2)) -eq 0 ] && arr[$char]=$double
\$\endgroup\$
3
\$\begingroup\$

Does this code follow good practices?

It has some violations.

Avoid sub-shells when you don't need one

Instead of this:

str=$1
checknum=$(echo ${str: -1})

You can use parameter expansion (${...}) directly without a sub-shell:

str=$1
checknum=${str: -1}

The same goes for the similar use in the getrest function.

Arithmetic operations

Instead of this:

while [ "$i" -lt "${#str}" ]; do
    arr+=(${str:$i:1})
    i=$((i+1))
done

It's better to use ((...)) for arithmetic operations, like this:

while (( i < ${#str} )); do
    arr+=(${str:$i:1})
    ((++i))
done

Empty then in if statements

I see you added a dummy assignment foo=bar in this if statement because the then clause cannot be empty:

if [ $(($i%2)) -eq 0 ] ; then foo=bar; else arr[$char]=$double; fi

In such situation, it's better to negate the condition:

if ! [ $(($i%2)) -eq 0 ] ; then arr[$char]=$double; fi

Odd groupings

The grouping with {...} is strange and unnecessary here:

while condition
do {
    # ...
}
done

Unless you need the {...} for something, then it's better to write simpler:

while condition
do
    # ...
done

Avoid echo -e

The various flags of echo are not portable. Avoid them when possible.

The command echo -e without other arguments is exactly the same as echo. So just write echo and that's it.

\$\endgroup\$
2
\$\begingroup\$

In addition to Janos' points (which are all very good points) it should be noted that variables in Bash are untyped (in other words - a string that is made up of numerics can be used in arithmetic). So the conversion from string to array is unnecessary.

With that in mind (and because I found this question whilst looking for a Bash implementation of the Luhn check) I thought I'd provide what I ended up using:

#!/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

This script takes a single parameter (the PAN) and exits with 1 if the PAN is invalid. For example, if we place this script in a file called isLUHNValid.sh we can run it 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'.

\$\endgroup\$
  • \$\begingroup\$ Good point about untyped variables, and welcome to Code Review. Btw if you post your version up for review, I'll have a few comments on it ;-) \$\endgroup\$ – janos May 27 '16 at 7:45
  • \$\begingroup\$ Thanks. I'd actually love feedback. Should I make a new post? Being new here I'm not really sure of the shoulds and should nots. If I make a new post - do I just say "Here's a script I wrote. Looking for feedback"? \$\endgroup\$ – Brendan McGrath May 27 '16 at 11:54
  • \$\begingroup\$ Something like that, but do read the how-to-ask and on-topic pages first \$\endgroup\$ – janos May 27 '16 at 12:03
  • \$\begingroup\$ Thanks again Janos. I've posted my version here: codereview.stackexchange.com/questions/129649/… \$\endgroup\$ – Brendan McGrath May 30 '16 at 4:35

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.