What can be improved on my first haskell program?

Question

Here is my first Haskell program. What parts would you write in a better way?

-- Multiplication table
-- Returns n*n multiplication table in base b

import Text.Printf
import Data.List
import Data.Char

-- Returns n*n multiplication table in base b 
mulTable :: Int -> Int -> String
mulTable n b = foldl (++) (verticalHeader n b w) (map (line n b w) [0..n])
               where 
                 lo = 2* (logBase (fromIntegral  b)  (fromIntegral n))
                 w = 1+fromInteger (floor lo)

verticalHeader :: Int -> Int -> Int -> String  
verticalHeader n b w = (foldl (++) tableHeader columnHeaders)
                        ++ "\n" 
                        ++ minusSignLine 
                        ++ "\n"
                   where
                     tableHeader = replicate (w+2) ' '
                     columnHeaders = map (horizontalHeader b w) [0..n]
                     minusSignLine = concat ( replicate ((w+1)* (n+2)) "-" )

horizontalHeader :: Int -> Int -> Int -> String
horizontalHeader b w i = format i b w

line :: Int -> Int -> Int -> Int -> String
line n b w y  = (foldl (++) ((format y b w) ++ "|" ) 
                           (map (element b w y) [0..n])) ++ "\n"

element :: Int -> Int -> Int -> Int -> String  
element b w y x = format  (y * x) b w

toBase :: Int -> Int -> [Int]
toBase b v = toBase' [] v where
  toBase' a 0 = a
  toBase' a v = toBase' (r:a) q where (q,r) = v `divMod` b

toAlphaDigits :: [Int] -> String
toAlphaDigits = map convert where
  convert n | n < 10    = chr (n + ord '0')
            | otherwise = chr (n + ord 'a' - 10)

format :: Int -> Int -> Int -> String
format v b w = concat spaces ++ digits ++ " "
               where 
                   digits  = if v == 0 
                             then "0" 
                             else toAlphaDigits ( toBase b v )
                   l = length digits
                   spaceCount = if (l > w) then  0 else (w-l) 
                   spaces = replicate spaceCount " " 

Answer 1

0) add a main function :-) at least rudimentary

import System.Environment (getArgs)
import Control.Monad (liftM)

main :: IO ()
main = do
  args <- liftM (map read) $ getArgs
  case args of
    (n:b:_) -> putStrLn $ mulTable n b
    _       -> putStrLn "usage: nntable n base"

1) run ghc or runhaskell with -Wall; run through hlint.

While hlint doesn't suggest anything special here (only some redundant brackets), ghc will tell you that you don't actually need Text.Printf here...

2) try running it with base = 1 or base = 0 or base = -1

Answer 2

If you want multiline comments use:

{-  A multiline
   comment -}

Also, never use foldl, use foldl' instead, in cases where you are dealing with large lists which must be folded. It is more memory efficient.

Answer 3

A brief comments saying what each function does, its arguments and return value, is always good. I had to read the code pretty carefully to fully make sense of it.

Some would say if you do that, explicit type signatures may not be required. That's an aesthetic question, I don't have a strong opinion on it.

One minor caveat: if you do remove the type signatures, you'll automatically get the polymorphic Integral support ephemient mentioned, but you will still need one around toAlphaDigits because of the infamous "monomorphism restriction."

Answer 4

Norman Ramsey gave excellent high-level (design) suggestions; Below are some low-level ones:

• First, consult with HLint. HLint is a friendly program that gives you rudimentary advice on how to improve your Haskell code!
  • In your case HLint gives 7 suggestions. (mostly about redundant brackets)
  • Modify your code according to HLint's suggestions until it likes what you feed it.
• More HLint-like stuff:
  • concat (replicate i "-"). Why not replicate i '-'?
• Consult with Hoogle whenever there is reason to believe that a function you need is already available in Haskell's libraries. Haskell comes with tons of useful functions so Hoogle should come in handy quite often.
  • Need to concatenate strings? Search for [String] -> String, and voila you found concat. Now go replace all those folds.
  • The previous search also suggested unlines. Actually, this even better suits your needs. It's magic!
• Optional: pause and thank in your heart to Neil M for making Hoogle and HLint, and thank others for making other good stuff like Haskell, bridges, tennis balls, and sanitation.
• Now, for every function that takes several arguments of the same type, make it clear which means what, by giving them descriptive names. This is better than comments, but you can still use both.

So

-- Returns n*n multiplication table in base b 
mulTable :: Int -> Int -> String
mulTable n b =

becomes

mulTable :: Int -> Int -> String
mulTable size base =

• To soften the extra characters blow of the previous suggestion: When a function is only used once, and is not very useful by itself, put it inside its caller's scope in its where clause, where it could use the callers' variables, saving you the need to pass everything to it.

So

line :: Int -> Int -> Int -> Int -> String
line n b w y =
  concat
  $ format y b w
  : "|"
  : map (element b w y) [0 .. n]

element :: Int -> Int -> Int -> Int -> String  
element b w y x = format (y * x) b w

becomes

line :: Int -> Int -> Int -> Int -> String
line n b w y =
  concat
  $ format y b w
  : "|"
  : map element [0 .. n]
  where
    element x = format (y * x) b w

• You can even move line into mulTable's where clause; imho, you should.
  • If you find a where clause nested inside another where clause troubling, then I suggest to change your indentation habits. My recommendation is to use consistent indentation of always 2 or always 4 spaces. Then you can easily see, everywhere, where the where in the other where is at. ok

Below's what it looks like (with a few other changes in style):

import Data.List
import Data.Char

mulTable :: Int -> Int -> String
mulTable size base =
  unlines $
  [ vertHeaders
  , minusSignsLine
  ] ++ map line [0 .. size]
  where
    vertHeaders =
      concat
      $ replicate (cellWidth + 2) ' '
      : map horizontalHeader [0 .. size]
    horizontalHeader i = format i base cellWidth
    minusSignsLine = replicate ((cellWidth + 1) * (size + 2)) '-'
    cellWidth = length $ toBase base (size * size)
    line y =
      concat
      $ format y base cellWidth
      : "|"
      : map element [0 .. size]
      where
        element x = format (y * x) base cellWidth

toBase :: Integral i => i -> i -> [i]
toBase base
  = reverse
  . map (`mod` base)
  . takeWhile (> 0)
  . iterate (`div` base)

toAlphaDigit :: Int -> Char
toAlphaDigit n
  | n < 10    = chr (n + ord '0')
  | otherwise = chr (n + ord 'a' - 10)

format :: Int -> Int -> Int -> String
format v b w =
  spaces ++ digits ++ " "
  where 
    digits
      | v == 0    = "0"
      | otherwise = map toAlphaDigit (toBase b v)
    spaces = replicate (w - length digits) ' '