requires / or \ at end.
Your code requires that the user supplied source and destination
directories end in a path name separator. A better approach is
to simply add it. See the comments below on the
When processing the command line arguments you
are using functions which could throw errors, e.g.
When they fail you'll get a generic error message like
Prelude.read: no parse or
Prelude.!!: index too large.
Hackage has libraries you can use to parse and validate command line arguments
like cmdargs or any of the optparse- packages. However, it's not too difficult to roll your own like this:
parseArgs (nstr : source : dest : _) =
if all isDigit nstr
then ( read nstr, source, dest )
else error "first argument is not a number"
parseArgs _ = error "not enough arguments"
main = do
args <- getArgs
let (n, source, dest) = parseArgs args
parseArgs checks that there are at least three command line arguments and
also that the first one consists entirely of digits.
FilePath type is a
String, so using
(++) to join
together path name segments is fine. However, in System.FilePath
Haskell defines the operator
(</>) which is meant to be a cross-platform
way to do this - i.e. on Windows it will use
\\ and on Posix systems it
will use a
/. Moreover using
(</>) signals to the reader that you are
working with a path name string.
System.FilePath also has utility functions for dealing with
path names such as extracting the base / directory name from a path, extracting and replacing the suffix, etc. which are good to know about and may come
A good place to use
(</>) is in your
listFilesInDir function, e.g.:
listFilesInDir dir =
getDirectoryContents >>= filterM (\x -> doesFileExist (dir </> x))
dir does not have to end in a
\\ because using
add the appropriate separator.
fileList is an IO-action, so in these lines:
fileSources <-fmap (map (source ++)) $ fileList
fileSinks <-fmap (map (sink ++)) $ fileList
you are repeating the IO-action twice. That is, you are performing
getDirectoryContents twice on the same directory. Instead you
want to perform
only once and use its result to compute
leafNames <- listFilesInDir source
let fileSources = map (source </>) leafNames
fileSinks = map (sink </>) leafNames
zipWithM_ (writeFirstNLines n) fileSources fileSinks
Note I'm using
zipWithM_ here - it works just like
zipWithM but discards
the results. It's more efficient because
zipWithM_ discards the result of
the zip operation immediately, whereas
zipWithM saves the results in a
list thus consuming memory for data you'll never look at.
In Haskell you need to explicitly close your file handles when you are done
with them. Every process has limit on the number of handles which can be open at any one time, so if you never close any handles then eventually
openFile will fail when you hit this limit.
A basic way to make sure your file handles always get closed is to use
withFile :: FilePath -> IOMode -> (Handle -> IO r) -> IO r
withFile creates a file handle by opening a path with the specified IOMode
and passes the handle to an IO-action. It will ensure that the handle is
closed when the IO-action completes - whether it returns normally
or if an exception is thrown.
writeFirstNLines would look like:
writeFirstNLines n source sink = do
withFile source ReadMode (\hin -> do
withFile sink WriteMode (\hout -> do
... copy from hin to hout ...
hout will automatically get closed when the copy action
replicateM_ n action will always execute
n times - no more and no less. So this is not a good way
to write a loop which could be executed a variable number of times.
As an example of how inefficient using
replicateM_ here could be,
n is 1000 and you are operating on a 3 line file.
replicateM_ n readAndWrite
readAndWrite 1000 times even though the last
997 calls won't copy any lines.
Here is a recipe for a loop which is executed at most
with the possibility of breaking out early:
loop n | n <= 0 = return ()
loop n = do quit <- condition -- check condition
then return () -- break out early
else ... do stuff...
loop (n-1) -- perform next iteration
You can use this to copy the first n lines by substituting:
condition = hIsEOF hin
...do stuff... = hGetLine hin >>= hPutStrLn hout