Please tell me how I can improve the structure of this simple tool (depends on MissingH):

module Main (main) where

import Data.List(isInfixOf)
import Data.String.Utils(replace)
import System.Environment(getArgs)
import System.Directory(renameFile, getDirectoryContents)

pathsContaining :: String -> [String] -> [String]
pathsContaining r ps = filter (isInfixOf r) ps

pathsReject     :: String -> [String] -> [String]
pathsReject r ps = map (replace r "") ps

pathsRejected   :: String -> [String] -> [String]
pathsRejected r = (pathsReject r) . (pathsContaining r)

addBaseToPaths  :: String -> [String] -> [String]
addBaseToPaths b p = map (\x -> b ++ "/" ++ x) p

getDeltas       :: String -> String -> [String] -> [(String, String)]
getDeltas b r ps = let o = addBaseToPaths b $ pathsContaining r ps
                       d = addBaseToPaths b $ pathsRejected   r ps
                       in zip o d

renameFromPairs :: [(FilePath, FilePath)] -> IO()
renameFromPairs [] = return ()
renameFromPairs (x:xs) = do
  (uncurry renameFile) x
  renameFromPairs xs

main :: IO()
main = do
  [directory, reject] <- getArgs
  files               <- getDirectoryContents directory
  let deltas = getDeltas directory reject files
  renameFromPairs deltas
  putStrLn $ (show . length $ deltas) ++ " files renamed"

That seems pretty good. I would probably not write pathsReject and pathsContaining since you only use them once and their definition is as short as their name and very easy to understand (I don't see how naming map (replace r "") ps makes code easier to understand, the name is not obvious (and has nothing to do with rejecting paths)). I would also use isInfixOf\ infix since it reads more naturally so :

getDeltas :: String -> [String] -> [(String, String)]
getDeltas r ps = let o = filter (r `isInfixOf`) ps
                     d = map (replace r "") o
                 in zip o d

renameDeltaInBase :: String -> (String,String) -> IO ()
renameDeltaInBase b (o,d) = renameFile (b </> o) (b </> d)

(Import </> from System.FilePath to ensure your code works on all OS).

Also your renameFromPairs is unnecessary, simply use mapM_ (map to apply to all elements of a list, M in a monad, _ returning ()) imported from Control.Monad :

main :: IO()
main = do
  [directory, reject] <- getArgs
  files               <- getDirectoryContents directory
  let deltas = getDeltas reject files
  mapM_ (renameDeltaInBase directory) deltas
  putStrLn $ (show . length $ deltas) ++ " files renamed"
| improve this answer | |
  • \$\begingroup\$ Thank you so much, learning Haskell has been wonderful, but without human review its hard to know what the pragmatic approach is. Also, I am looking into testing with hspec, can you tell me how you would go about testing this code? \$\endgroup\$ – Fresheyeball May 23 '14 at 23:17

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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