Aligning text in Haskell

Question

This Haskell program just aligns the text lines on the given substrings.

I took inspiration from here, but my code is not golfed.

import Data.Function
import Data.Ord
import Data.List
import Data.List.Split
alignOn :: String -> [String] -> [String]
alignOn char lines = map padline lines
 where 
 partBeforechar = head . splitOn char
 longestLengthBeforeChar = maximum $ map (length . partBeforechar) lines
 padline line = replicate offset ' ' ++ line
 where
 offset = longestLengthBeforeChar - (length (partBeforechar line))
main = do
 mapM putStrLn $ alignOn "," ["Programming, Puzzles", "And, Code Golf"]
 mapM putStrLn $ alignOn ", " ["Code, Review", "And Other, improvements"]
 mapM putStrLn $ alignOn "abc" ["Example abc bar", "foo abc Example"]

Output:

Programming, Puzzles
 And, Code Golf
 Code, Review
And Other, improvements
Example abc bar
 foo abc Example

• zigazou: Can you specify the kind of code review you’re waiting for this code ? Code formatting, speed optimization, tips and tricks, functional approach... ?
• Caridorc: @zigazou anything.

Answer 1

Use -Wall

When you are evaluating your code, you should use Haskell -Wall verbosity.

ghc -Wall mycode.hs

or

runghc -Wall mycode.hs

For example, it will tell you:

• Unneeded imports (the import of ... is redundant)
• Inappropriate use of variable names (this binding for ... shadows the existing binding)
• Missing type signatures (top-level binding with no type signature ...)
• etc.

This is generally good advice.

Specifying imported functions and types in import statements helps future reading of your code to determine why a particular import is needed.

For example:

import Data.List.Split (splitOn)

Use hlint

hlint is another useful tool to help you improve your code. For example, it would tell you that parentheses are unnecessary in the following case:

longestLengthBeforeChar - (length (partBeforechar line))

That could be replaced by:

longestLengthBeforeChar - length (partBeforechar line)

Or that, you shouldn’t use mapM if you don’t intend to use the result. You should use mapM_ (note the underscore) to make it clear.

Again, like -Wall, hlint suggestions are generally good advice you should follow.

Factorize

When you often see a pattern in your code, it may be an alert that it needs factorization.

For example:

mapM putStrLn $ alignOn "," ["Programming, Puzzles", "And, Code Golf"]
mapM putStrLn $ alignOn ", " ["Code, Review", "And Other, improvements"]
mapM putStrLn $ alignOn "abc" ["Example abc bar", "foo abc Example"]

could be replaced by:

let tests = [ ("," , ["Programming, Puzzles", "And, Code Golf"])
 , (", " , ["Code, Review", "And Other, improvements"])
 , ("abc", ["Example abc bar", "foo abc Example"])
 ]
mapM_ (putStr . unlines . uncurry alignOn) tests

(better: use QuickCheck and put the test code outside your module)

Naming convention

You should use appropriate names for your parameters. Naming char a parameter which is in fact a String is misleading. It also does not give any indication on the role of this parameter. It could be renamed separator, for example.

You should also avoid to use a name that will shadow another one. You use lines in your function as a parameter, but lines is a well defined function (the counterpart of unlines).

Warning

Some Haskell functions like head can generate exceptions:

head [] -- Exception: Prelude.head: empty list

When you use it in your code, you should ensure an empty list will never occur (since it comes right after the splitOn function, it is safe to use it).

Coding convention

Avoid a where clause inside another where clause. It generally indicates the code should be split because your function is complex. Complex functions are harder to debug.

In fact, in your code, the alignOn can be splitted into independent, reusable and testable functions.

For example:

alignOn :: String -> [String] -> [String]
alignOn sep rows = fmap padline rows
 where padding row = longestLengthBefore sep rows - lengthBefore sep row
 padline row = replicate (padding row) ' ' ++ row
lengthBefore :: String -> String -> Int
lengthBefore sep = length . head . splitOn sep
longestLengthBefore :: String -> [String] -> Int
longestLengthBefore sep = maximum . fmap (lengthBefore sep)

Note:

• This example is NOT optimized.
• fmap should be preferred to map as it is more general.

Generalizing

The padding character could be defined as a parameter.

Optimization

You could rewrite your code to use Text instead of String.

Answer 2

You only ever use the length of partBeforechar, so you should move the (length .) into its definition. I don't like the name char and would replace it with something, say, derived from the word delimiter. Consider replacing mapM with unlines later in the line.