7
\$\begingroup\$

There is some little program (using a Last.FM API wrapper) which gets the play count of 2 specific users every 5 minutes and appends the difference between these 2 numbers to log file.

Can you review that part of the code? I think that using the <- operator so many times decreases the readability and simplicity of the code.

main = forever $ do
  diff <- user1 `subtractCounts` user2
  timestamp <- formatTime defaultTimeLocale dateFormat <$> getZonedTime
  let logMessage = printf "%s %d\n" timestamp diff :: String
  homeDir <- getHomeDirectory
  let logFilePath = homeDir </> logFile
  appendFile logFilePath logMessage
  threadDelay $ 10^6 * 60 * timeDelay -- in microseconds
    where
      subtractCounts = liftM2 ((-) `on` fromMaybe 0) `on` getPlayCount
      dateFormat = "[%s] %D %H:%M"
\$\endgroup\$
10
\$\begingroup\$

I don't see why you say <- "decreases readability", but you could line them up if you think that helps (this style is typical of Haskell programmers)

main = forever $ do
  diff      <- user1 `subtractCounts` user2
  timestamp <- formatTime defaultTimeLocale dateFormat <$> getZonedTime
  homeDir   <- getHomeDirectory
  let logMessage  = printf "%s %d\n" timestamp diff
      logFilePath = homeDir </> logFile
  appendFile logFilePath logMessage
  threadDelay $ 10^6 * 60 * timeDelay -- in microseconds

subtractCounts = liftM2 ((-) `on` fromMaybe 0) `on` getPlayCount
dateFormat     = "[%s] %D %H:%M"

<- is a visual cue that the thing on the right might to perform an effect before producing a result and binding it to the name on the left.

Note that the type annotation on logMessage is unnecessary. Because it is used as the second argument to appendFile it will be inferred to be String.

I personally might make a few more refactorings, but they aren't really necessary:

logMessage :: String -> Integer -> String
logMessage  = printf "%s %d\n"

(f .: g) x y = f $ g x y

subtractCounts = liftM2 ((-) `on` fromMaybe 0) `on` getPlayCount


main = forever $ do
  homeDir <- getHomeDirectory
  let logFilePath = homeDir </> logFile
      logDiff     = appendFile logFilePath .: logMessage
  forever $ mainLoop logDiff

mainLoop logDiff = do
  diff      <- user1 `subtractCounts` user2
  timestamp <- formatTime defaultTimeLocale "[%s] %D %H:%M" <$> getZonedTime
  logDiff timestamp diff
  threadDelay $ 10^6 * 60 * timeDelay -- in microseconds

The type signature on logMessage is probably unnecessary, but imho it's a good idea to give it an explicit signature if you are using the result of printf as a function, rather than a String or IO ().

\$\endgroup\$
  • \$\begingroup\$ Where is .: operator from? \$\endgroup\$ – ДМИТРИЙ МАЛИКОВ Mar 26 '12 at 18:35
  • \$\begingroup\$ Oh, sorry, I forgot to mention: (f .: g) x y = f (g x y). It's not in the standard libraries, but is a common trick for writing pointfree code. \$\endgroup\$ – Dan Burton Mar 26 '12 at 19:28
  • \$\begingroup\$ Actually you forgot about threadDelay. \$\endgroup\$ – ДМИТРИЙ МАЛИКОВ Mar 26 '12 at 20:01

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.