current community

your communities

more stack exchange communities

Stack Exchange
tour help
Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 0 down vote favorite

My implementation in Progress 4GL of the Luhn Algorithm. Any suggestions on improving it?

FUNCTION fnLuhnAlgorithm RETURNS LOGICAL
  (INPUT pcNumber AS CHARACTER):
/*------------------------------------------------------------------------------
  Purpose:  Applies Luhn Algorithm to check a Number 
    Notes:  Returns True/False Validation based on check digit

    From the rightmost digit, which is the check digit, moving left, double the value
    of every second digit; 
    if product of this doubling operation is greater than 9 (e.g., 7 * 2 = 14).
        Sum the digits of the products (e.g., 10: 1 + 0 = 1, 14: 1 + 4 = 5) together 
    Compute the sum of the digits.
    Multiply by 9.
    The last digit, is the check digit.
------------------------------------------------------------------------------*/
    DEFINE VARIABLE cNum        AS CHARACTER NO-UNDO.
    DEFINE VARIABLE iCheck      AS INTEGER   NO-UNDO.
    DEFINE VARIABLE iLength     AS INTEGER   NO-UNDO.
    DEFINE VARIABLE iLoopCnt    AS INTEGER   NO-UNDO.
    DEFINE VARIABLE iNum        AS INTEGER   NO-UNDO.
    DEFINE VARIABLE iNum1       AS INTEGER   NO-UNDO.
    DEFINE VARIABLE iNum2       AS INTEGER   NO-UNDO.
    DEFINE VARIABLE iTestLength AS INTEGER   NO-UNDO.

    ASSIGN 
      iLength     = LENGTH(pcNumber)
      iTestLength = iLength - 1
        iCheck = 1. /* 1 for the check digit we skip */

    DO iLoopCnt = iTestLength TO 1 BY -1:
      ASSIGN 
          iNum = INTEGER(SUBSTR(pcNumber,iLoopCnt,1))
          iCheck = iCheck + 1.

      IF iCheck MODULO 2 = 1 THEN
          ASSIGN iNum1 = iNum1 + iNum.
      ELSE
      DO:
          ASSIGN iNum2 = iNum * 2.
          IF iNum2 < 10 THEN
              ASSIGN iNum1 = iNum1 + iNum2.
          ELSE
              ASSIGN
                cNum  = STRING(iNum2)
                iNum1 = iNum1 + INTEGER(SUBSTR(cNum,1,1)) + INTEGER(SUBSTR(cNum,2,1)).   
      END.
    END.

    ASSIGN 
        iNum2 = iNum1 * 9
        iNum = iNum2 MODULO 10.

    IF iNum = INTEGER(SUBSTR(pcNumber,iLength,1)) THEN
      RETURN TRUE.
    ELSE
      RETURN FALSE.

END FUNCTION. /* fnLuhnAlgorithm  */
share|improve this question

1 Answer 1

up vote 0 down vote accepted

You are kind of overdoing it a little in my opinion, there are too many variables that don't really add much value, also I would change the parameter to be DECIMAL, to avoid calls using not numbers that would cause run time errors.

My implementation would look like this:

FUNCTION fnLuhnAlgorithm RETURNS LOGICAL
  (INPUT pcNumber AS DECIMAL):
/*------------------------------------------------------------------------------
  Purpose:  Applies Luhn Algorithm to check a Number 
    Notes:  Returns True/False Validation based on check digit

    From the rightmost digit, which is the check digit, moving left, double the value
    of every second digit; 
    if product of this doubling operation is greater than 9 (e.g., 7 * 2 = 14).
        Sum the digits of the products (e.g., 10: 1 + 0 = 1, 14: 1 + 4 = 5) together 
    Compute the sum of the digits.
    Multiply by 9.
    The last digit, is the check digit.
------------------------------------------------------------------------------*/
    DEFINE VARIABLE cNumber     AS CHARACTER NO-UNDO INITIAL "".
    DEFINE VARIABLE iDigit      AS INTEGER   NO-UNDO INITIAL 0.
    DEFINE VARIABLE iSum        AS INTEGER   NO-UNDO INITIAL 0.
    DEFINE VARIABLE iLoopCnt    AS INTEGER   NO-UNDO INITIAL 0.

    cNumber = STRING(pcNumber).

    DO iLoopCnt = LENGTH(cNumber) - 1 TO 1 BY -1:

      iDigit = INTEGER(SUBSTR(cNumber,iLoopCnt,1)).

      IF iLoopCnt MODULO 2 = LENGTH(cNumber) MODULO 2 THEN
          iSum = iSum + iDigit.
      ELSE
          iSum = iSum + INTEGER(SUBSTR(STRING(iDigit * 2,"99"),1,1)) 
                      + INTEGER(SUBSTR(STRING(iDigit * 2,"99"),2,1)).   

    END.

    IF ((iSum * 9) MODULO 10) = INTEGER(SUBSTR(cNumber,LENGTH(cNumber),1)) THEN
      RETURN TRUE.
    ELSE
      RETURN FALSE.

END FUNCTION. /* fnLuhnAlgorithm  */
share|improve this answer
    
Looks good, I would just either keep the input parm as character or make it integer instead of decimal. –  AquaAlex Jun 21 '13 at 11:23
    
if input parm is character you can also get rid of the variable cNumber –  AquaAlex Jun 21 '13 at 11:52
    
I have defined as DECIMAL because it would be able to hold a much higher maximum value than an INTEGER, and if you are dealing with account numbers or similar you'll reach it easily. About the CHARACTER input parameter, if that is the case then you'll need to first check the value is a valid number to be able to check it, also, seems much more clear if the function is to validate numbers that the parameter you pass is a number itself to me. –  pedromarce Jun 21 '13 at 20:46
    
Agree will need a check to make sure it is number, but same if input is decimal you still need to check to make sure it is not a decimal value. I use the LUHN Algorithm to test ID number, but before I call the algorithm i first do a lot of tests to make sure the ID number is a number and contains valid values in certain positions, etc. So by the time I call the function I know I have a valid number I just need to make sure the check digit is correct, so the Luhn Algorithm does not have to worry if value is a number. –  AquaAlex Jun 26 '13 at 10:40

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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