UPenn CIS 194 Homework 1: Validating credit card numbers

I am working through UPenn CIS 194: Introduction to Haskell (Spring 2013). Since I am not able to take the course for real I am asking for CR (feedback) as it could be from teacher in that course.

HW1 - Validating Credit Card Numbers - Full description

A shortened problem statement:

Validate a credit card number by the following steps:

• Double the value of every second digit from the right

• Take the sum of the digits of the new values

• Check whether the sum modulo 10 is 0.

Write the functions toDigits, toDigitsRev and doubleEveryOther for the first task, sumDigits for the second, and validate for the third.

toDigitsRev :: Integer -> [Integer]
toDigitsRev 0 = []
toDigitsRev n
| n [Integer]
toDigits n = reverse (toDigitsRev n)

doubleEveryOther :: [Integer] -> [Integer]
doubleEveryOther [] = []
doubleEveryOther (x:[]) = [x]
doubleEveryOther (x:y:zx)
| length (x:y:zx) mod 2 == 0 = 2*x : y : doubleEveryOther zx
| otherwise = x : 2*y : doubleEveryOther zx

sumDigits :: [Integer] -> Integer
sumDigits [] = 0
sumDigits (x:[]) = x
sumDigits (x:xs)
| x Bool
validate 0 = False
validate n = (sumDigits (doubleEveryOther (toDigits n))) mod 10 == 0

type Peg = String
type Move = (Peg, Peg)
hanoi :: Integer -> Peg -> Peg -> Peg -> [Move]
hanoi n a b c
| n == 0 = []
| n == 1 = (a, b) : hanoi 0 a b c
| otherwise = hanoi (n-1) a c b ++ [(a, b)] ++ hanoi (n-1) c b a


You're on the right track, I think. If I had to pick one general criticism, it would be to watch out for redundant special cases.

In my opinion, the exercise guides you towards making an unnecessary complication in toDigits and doubleEveryOther. Actually, the list of digits from right to left, as produced by toDigitsRev, is the more natural representation to work with. If you work with the list in left-to-right order, then doubleEveryOther needs to look ahead to see whether the list has an odd or even digit count, which is awkward. Therefore, if given free reign, I'd define a doubleEveryOther' that expects right-to-left input instead.

toDigitsRev does not need special cases for 0 and for n < 10. Instead of calling div and mod, use the divMod function to capture both pieces of information from the same calculation.

toDigitsRev :: Integer -> [Integer]
toDigitsRev n
  | n <= 0    = []
  | otherwise = m : toDigitsRev d
  where
    (d, m) = n `divMod` 10

As mentioned above, we can simplify doubleEveryOther by working right-to-left:

-- Doubles the second, fourth, sixth… numbers in the list
doubleEveryOther' :: [Integer] -> [Integer]
doubleEveryOther' (x:y:zs) = x : 2 * y : doubleEveryOther' zs
doubleEveryOther' (x:[])   = [x]
doubleEveryOther' []       = []

You don't need a case for sumDigits (x:[]) — it's already covered by sumDigits (x:xs), where xs is the empty list. The way you wrote it is fine, but personally I'd prefer to write it as sum . map sumDigits' so that you can see at a glance that it's performing a sum of some transformed values.

sumDigits :: [Integer] -> Integer
sumDigits = sum . map sumDigits'
  where
    sumDigits' n
      | n < 10    = n
      | otherwise = sumDigits $ toDigitsRev n

The validate function does not need a special case for 0. It's also more idiomatic to set up a "pipeline" of function calls using $ than to use nested parentheses.

validate :: Integer -> Bool
validate n = 0 == (sumDigits $ doubleEveryOther' $ toDigitsRev n) `mod` 10

StackExchange Code Review Q#104876, answer score: 4