Problem

I’ve tried to solve a challenge posted on a LinkedIn forum using Haskell – a language I’m still learning the basics of – and, while the code works correctly, I would like to get some feedback on the coding style, and learn what could be improved.

The task is to add all Armstrong numbers within a range.

An Armstrong (or narcissistic) number is a number that is equal to the sum of all its digits, each raised to the power of the length of its digits.

For instance, 153 is an Armstrong number because 13+53+33

equals 153.

The range can be given in any order – that is `5 10`

and `10 5`

denote the same interval, that is, all numbers between 5 and 10 (boundaries included).

And here’s my code:

```
digits :: (Integral n) => n -> [n]
digits n
| n < 10 = [n]
| otherwise = digits (n `div` 10) ++ [n `mod` 10]
numberLength :: Int -> Int
numberLength = length . digits
powerNumber :: Int -> [Int]
powerNumber n = map (d -> (d ^ exponent)) listOfDigits
where exponent = numberLength n
listOfDigits = digits n
isArmstrong :: Int -> Bool
isArmstrong a = a == sum (powerNumber a)
sortEnds :: Int -> Int -> [Int]
sortEnds a b = if a < b then [a, b] else [b, a]
sumAllArmstrongNumber :: Int -> Int -> Int
sumAllArmstrongNumber a b = sum([x | x <- [start..end], isArmstrong x])
where [start, end] = sortEnds a b
```

Any feedback is much appreciated!

Solution

I would like to get some feedback on the coding style, and learn what could be improved.

First of all, it’s fantastic that all your functions have proper types, consistent indentation and style. Keep that in your future code style!

# Prefer `divMod`

or `quotRem`

If you use both `y `div` x`

and `y `mod` x`

for the same `x`

and `y`

then you should use `divMod`

instead. Even better, use `quotRem`

if possible. `quotRem`

returns the same results for positive numbers but is slightly faster:

```
digits :: (Integral n) => n -> [n]
digits n
| n < 10 = [n]
| otherwise = let (q, r) = n `quotRem` 10
in digits q ++ [r]
```

# Cons; don’t append

There’s one big drawback in the new `digits`

function though: we’re always appending a single element. This yields an O(n2)

algorithm, since `(x:xs) ++ [y] = x : (xs ++ [y])`

. Instead, we should cons the new element on the rest of the list:

```
digits :: (Integral n) => n -> [n]
digits n
| n < 10 = [n]
| otherwise = let (q, r) = n `quotRem` 10
in r : digits q
```

Note that this will return the digits in reverse order but that’s fine. If you want the digits in the original order, consider using `reverse`

on the result:

```
digits :: (Integral n) => n -> [n]
digits = reverse . go
where go n = case n `quotRem` 10 of
(0, r) -> [r]
(q, r) -> r : go q
```

I’m a fan of `case`

for this style of `quotRem`

usage, but that’s just my personal opinion.

# Map vs pointfree vs comprehension

*Warning: this section and mostly about personal preference.*

In the following code, `listOfDigits`

and `expontent`

are defined and used once:

```
``````
powerNumber :: Int -> [Int]
powerNumber n = map (d -> (d ^ exponent)) listOfDigits
where exponent = numberLength n
listOfDigits = digits n
```

I’d personally prefer `digits n`

instead of `listOfDigits`

, since it’s only used as an argument for `map`

:

```
powerNumber n = map (d -> (d ^ exponent)) $ digits n
where exponent = numberLength n
```

Next `(d -> (d ^ exponent))`

is `(^exponent)`

, which is preferred by some:

```
powerNumber n = map (^exponent) $ digits n
where exponent = numberLength n
```

But in terms of readability, a list comprehension might even better:

```
powerNumber :: Int -> [Int]
powerNumber n = [ d ^ exponent | d <- digits n ]
where exponent = numberLength n
```

# Add additional information in return types

```
``````
sortEnds :: Int -> Int -> [Int]
sortEnds a b = if a < b then [a, b] else [b, a]
```

The return type is slightly misleading: `sortEnds`

always returns exactly two elements. So we should use a pair:

```
sortEnds :: Int -> Int -> (Int, Int)
sortEnds a b = if a < b then (a, b) else (b, a)
```

# Remove superfluous parentheses

```
``````
sumAllArmstrongNumber :: Int -> Int -> Int
sumAllArmstrongNumber a b = sum([x | x <- [start..end], isArmstrong x])
where [start, end] = sortEnds a b
```

I’d remove the explicit parentheses around `sum`

‘s argument:

```
sumAllArmstrongNumber :: Int -> Int -> Int
sumAllArmstrongNumber a b = sum [x | x <- [start..end], isArmstrong x]
where (start, end) = sortEnds a b
```