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:
- readable, elegant function?
- efficient and optimized function?
-
\$\begingroup\$ Why did you alter it? What was wrong with the original? \$\endgroup\$Mast– Mast ♦2018年10月28日 18:20:59 +00:00Commented 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\$Bear Bile Farming is Torture– Bear Bile Farming is Torture2018年10月29日 01:22:54 +00:00Commented Oct 29, 2018 at 1:22
1 Answer 1
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
andmapMaybe
)