1
\$\begingroup\$
isLineTerminator c = c == '\r' || c == '\n'
leadWords [] = []
leadWords cs =
    let (pre, suf) = break isLineTerminator cs
    in  (head (words pre)) : case suf of 
                            ('\r':'\n':rest) -> leadWords rest
                            ('\r':rest)      -> leadWords rest
                            ('\n':rest)      -> leadWords rest
                            _                -> []

fixLines :: String -> String
fixLines input = unlines (leadWords input)

interactWith function inputFile outputFile = do
    input <- readFile inputFile
    writeFile outputFile (function input)

main = mainWidth myFunction
  where mainWidth function = do
            args <- getArgs
            case args of 
                [input, output] -> interactWith function input output
                _               -> putStrLn "error: exactly two arguments needed"
        myFunction = fixLines

Much of this program is written by the author of the book on Haskell that I'm reading, so I assume that much of it is good. However, my part is in altering the leadWords function. Does it look like a:

  1. readable, elegant function?
  2. efficient and optimized function?
\$\endgroup\$
2
  • \$\begingroup\$ Why did you alter it? What was wrong with the original? \$\endgroup\$
    – Mast
    Oct 28, 2018 at 18:20
  • \$\begingroup\$ The original did not extract the first word. it just converted the lines from Windows formate to Linux format. \$\endgroup\$ Oct 29, 2018 at 1:22

1 Answer 1

2
\$\begingroup\$

Errors



The code block above is an example where your program will fail. Any empty line will lead to an empty list in words "" and therefore to head [], which calls error.

Standard library

That being said, there are some other issues with your code. You use unlines in fixLines, yet leadWords is essentially lines, although it also uses head . words internally. If we rewrite leadWords with map, we end up with

leadWords = map (head . words) . lines

which immediately shows the possible error on empty lines. Still, we can fix this if we write another function:

firstWord :: String -> Maybe String
firstWord xs = case words xs of
                   (x : _) -> Just x
                   _       -> Nothing

Now we can use mapMaybe from Data.Maybe to write leadWords:

leadWords :: String -> [String]
leadWords = mapMaybe firstWord . lines

Type signatures

Other than that, you should really add type signatures to your top-level functions. Only fixLines has one, and its not clear why that one got special handling.

Code complexity

main is overly complicated, too. We can just inline mainWidth, as its local binding doesn't yield any more comfort:

main :: IO ()
main =  do
    args <- getArgs
    case args of 
        [input, output] -> interactWith fixLines input output
        _               -> putStrLn "error: exactly two arguments needed"

Alternatively, we can provide mainWith as top level function and use that in main.

Complete code

If we apply all remarks given above, we end up with something similar to the following code:

import Data.Maybe (mapMaybe)

mainWith :: (String -> String) -> IO ()
mainWith f = do
    args <- getArgs
    case args of 
        [input, output] -> interactWith f input output
        _               -> putStrLn "error: exactly two arguments needed"

fixLines :: String -> String
fixLines = unlines . leadWords

main :: IO ()
main = mainWith fixLines

-- for completeness

leadWords :: String -> [String]
leadWords = mapMaybe firstWord . lines

firstWord :: String -> Maybe String
firstWord xs = case words xs of
                     (x : _) -> Just x
                     _       -> Nothing

-- That's just personal preference, your style is fine.
interactWith :: (String -> String) -> FilePath -> FilePath -> IO ()
interactWith f input output = readFile input >>= writeFile output . f

In short:

  • add type signatures to top-level functions
  • don't use head on lists that might be empty
  • use standard library types when possible (Maybe)
  • use standard library functions when possible (map, lines and mapMaybe)
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

Not the answer you're looking for? Browse other questions tagged or ask your own question.