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

Here are some suggestions:

• To make the tabularity of the computation more obvious, I would pass the list [0..n] to the line function rather than passing n.

• I would further split out the computation of the horizontal and vertical axes so that they are passed as arguments to mulTable rather than computed there.

• Haskell is higher-order, and almost none of the computation has to do with multiplication. So I would change the name of mulTable to binopTable and pass the actual multiplication in as a parameter.

• Finally, the formatting of individual numbers is repetitious. Why not pass \x -> format x b w as a parameter, eliminating the need for b and w?

The net effect of the changes I am suggesting is that you build a general higher-order function for creating tables for binary operators. Its type becomes something like

binopTable :: (i -> String) -> (i -> i -> i) -> [i] -> [i] -> String

and you wind up with a much more reusable function—for example, Boolean truth tables should be a piece of cake.

Higher-order and reusable is the Haskell Way.

Answer 2

You don't use anything from import Text.Printf.

Stylistically, you use more parentheses than necessary. Haskellers tend to find code more readable when it's cleaned of extraneous stuff like that. Instead of something like h x = f (g x), write h = f . g.

Nothing here really requires Int; (Integral a) => a ought to do.

foldl (++) x xs == concat $ x : xs: I trust the built-in concat to work better than your implementation.
Also, you should prefer foldr when the function is lazy in its second argument, as (++) is – because Haskell is lazy, this reduces stack space (and also works on infinite lists).
Also, unwords and unlines are shortcuts for intercalate " " and concat . map (++ "\n") respectively, i.e. "join with spaces" and "join with newlines (plus trailing newline)"; you can replace a couple things by those.

Unless you use big numbers, w = length $ takeWhile (<= n) $ iterate (* b) 1 is probably faster. Or, in the case of a lazy programmer, let w = length $ toBase b n.

concat ( (replicate ((w+1)* (n+2)) "-" ) == replicate ((w+1) * (n+2)) '-' – not sure how you missed this one, you got it right just a couple lines up.

You do the same thing with concat spaces, too. However, wouldn't it be easier to actually use the Text.Printf import and write printf "%*s " w digits?