When you are evaluating your code, you should use Haskell
ghc -Wall mycode.hs
runghc -Wall mycode.hs
For example, it will tell you:
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 ...)
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.
import Data.List.Split (splitOn)
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.
hlint suggestions are generally good advice you should follow.
When you often see a pattern in your code, it may be an alert that it needs
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
QuickCheck and put the test code outside your module)
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
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).
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.
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)
- This example is NOT optimized.
fmap should be preferred to
map as it is more general.
The padding character could be defined as a parameter.
You could rewrite your code to use
Text instead of