Dynamic IP-address logging

Question

Inspired by Don Stewart's article on Scripting with Haskell I tried a rewrite of a shell-script I did the other day. I was really astonished how easily it worked but as a whole it is about double the size of my shell script and some things feel a bit "clumsy".

• myReadTile and getCurlIP have IO-return type, which seems a bit "unhaskellish"
• extractIp and extractFromFile seem a bit unelegant especially the maybe parts and the trivial case "" to prevent last from throwing an error.

To run my code the packages curl network-info had to be installed - the rest was already there (Debian haskell-platform 201220.0, ghc-7.4.1)

What my code does:
I want to log the IP-address my provider gives me - and when it changes; as I have a dynamic IP. With this information I can ssh from other devices to my home PC, and I know how long the periods are in which my IP doesn't change.

Code

import System.IO (readFile, appendFile)
import System.Exit (exitSuccess)
import System.Directory (doesFileExist, getHomeDirectory)
import Data.Maybe (listToMaybe, fromMaybe)
import Data.Time (getZonedTime)
import Network.BSD (getHostName)
import Network.Curl (curlGetString, URLString, CurlOption(CurlTimeout))
import Network.Curl.Code (CurlCode(CurlOK))
import Network.Info (getNetworkInterfaces, NetworkInterface, name, ipv4)
localFilePath :: FilePath
localFilePath = "/Dropbox/iplog.log"
dyndns :: URLString
dyndns = "http://checkip.dyndns.org"
main :: IO ()
main = do
 home <- getHomeDirectory
 let iplogFilePath = home ++ localFilePath
 iplogS <- myReadFile iplogFilePath
 curlS <- getCurlIP dyndns
 let oldIP = extractFromFile iplogS
 currentIP = extractIp curlS
 if oldIP /= currentIP
 then do
 date <- fmap show getZonedTime
 host <- getHostName
 localIP <- fmap getLocalIP getNetworkInterfaces
 let content = unwords [currentIP, date, host, localIP, "\n"]
 appendFile iplogFilePath content
 else
 exitSuccess
extractFromFile :: String -> String
extractFromFile "" = ""
extractFromFile s = (fromMaybe [] . listToMaybe . words . last . lines) s
extractIp :: String -> String
extractIp "" = ""
extractIp s = (takeWhile (/='<') . last . words) s
myReadFile :: FilePath -> IO String
myReadFile fp = do
 iplogExists <- doesFileExist fp
 if iplogExists
 then
 readFile fp
 else do
 print "File does not exist - and will be created"
 print ("at: ~" ++ localFilePath)
 return ""
getCurlIP :: URLString -> IO String
getCurlIP url = do
 (curlState, curlString) <- curlGetString url [CurlTimeout 60]
 if curlState == CurlOK
 then
 return curlString
 else do
 print "No external IP found"
 return "0.0.0.0"
getLocalIP :: [NetworkInterface] -> String
getLocalIP = show . ipv4 . head . filter (\x -> name x == "eth0")

• Rahul Gopinath: Please do consider adding a unit test suite (HUnit or Quickcheck) for your script, it is a tremendous help when refactoring.

Answer 1

You have produced rather nice code, so my review is restricted to just the peripherals.

import System.IO (readFile, appendFile)
import System.Exit (exitSuccess)
import System.Directory (doesFileExist, getHomeDirectory)
import Data.Time (getZonedTime)
import Data.Char (isDigit, isSpace)
import Network.BSD (getHostName)
import Network.Curl (curlGetString, URLString, CurlOption(CurlTimeout))
import Network.Curl.Code (CurlCode(CurlOK))
import Network.Info (getNetworkInterfaces, NetworkInterface, name, ipv4)
import Control.Applicative (<$>, <*>)
localFilePath :: FilePath
localFilePath = "/myhaskell/iplog.log"
dyndns :: URLString
dyndns = "http://checkip.dyndns.org"
myEth = "eth0"

It is nicer to separate out self contained functions even if they are of type IO a (You already have some, but take out as much as you can.)

getLogPath :: IO String
getLogPath = flip (++) localFilePath <$> getHomeDirectory 

The warning that there are no external ips should really be elsewhere.

getCurlIP :: URLString -> IO String
getCurlIP url = extract <$> curlGetString url [CurlTimeout 60]
 where extract (CurlOK, str) = str
 extract _ = "0.0.0.0"
getLocalIP :: [NetworkInterface] -> String
getLocalIP = show . ipv4 . head . filter (flip (==) myEth . name)

Your ip extraction can be simplified further.

firstWord :: String -> String
firstWord = takeWhile (not . isSpace)
getFileIP = firstWord

I modified your implementation a bit to make it clear what is happening. We take letters while they are not numbers, and drop the suffix that start with '<' If your parsing goes beyond this, then go for parsec.

-- "<html><head><title>Current IP Check</title></head>
-- <body>Current IP Address: 24.20.128.212</body></html>\r\n"
extractIp :: String -> String
extractIp = takeWhile (/= '<') . dropWhile (not . isDigit)

Readfile shouldn't really say that a file if it does not exist will be created. It should just read the given file and return the value. The check and warning should be done elsewhere.

catFile :: FilePath -> IO String
catFile fp = doesFileExist fp >>= checkExist
 where checkExist True = readFile fp
 checkExist False = return []
checkS s [] = do 
 print ("File does not exist - and will be created\n" ++ "at: ~" ++ s)
 return []
checkS s str = return str

Using auxiliary definitions can make your code read much better. Prefer sequence to do notation when values are not used in intermediate computations. Try to move out of the imperative mindset when using do notation. (the do notation makes it easy to write 'c' in haskell :) ), using auxiliary functions can help you there. (I used case instead of if/then because that is what I prefer, there is no particular reason for that except that it makes it easy to use destructuring easier if I need to.)

main :: IO ()
main = do
 currentIP <- getCurrentIP
 iplogFilePath <- getLogPath
 oldIP <- getSavedIP iplogFilePath
 checkS iplogFilePath oldIP
 case oldIP /= currentIP of
 True -> getContent currentIP >>= appendFile iplogFilePath
 _ -> exitSuccess 
 where getContent ip = unwords <$> sequence [return ip, show <$> getZonedTime, getHostName,
 getLocalIP <$> getNetworkInterfaces]
 getSavedIP path = getFileIP <$> catFile path
 getCurrentIP = extractIp <$> getCurlIP dyndns

• epsilonhalbe: Thank you very much for your answer - that was exactly what I was looking for. Now I have to read through it and write more haskell ;-).