HiveBrain v1.2.0
Get Started
← Back to all entries
patternMinor

Network arithmetic, finding the next host for the given

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
thegivennexthostforfindingnetworkarithmetic

Problem

Disclaimer: It's literally the second time I write in Haskell.

The purpose of this module is to provide some basic operations with IP addresses (and networks).

At the moment I only implemented the successor operation, that returns a next address in the range if it exists.

Eg: for 192.168.1.1 in 192.168.1.0/24 it would return Maybe 192.168.1.1

And for 192.168.1.255 (which is the last valid IP address in the given network) it would return Nothing.

module Main where

import Data.IP (IPv4, AddrRange(addr), fromIPv4, toIPv4, isMatchedTo, makeAddrRange)
import Data.Bits (shiftL, shiftR, (.|.), (.&.))
import Data.List (foldl')
import Text.Read (readMaybe)
import qualified Safe as S

parse :: String -> Maybe IPv4
parse = readMaybe

toInt :: IPv4 -> Int
toInt i = foldl' accum 0 (fromIPv4 i)
    where accum a v = (a `shiftL` 8) .|. v

fromInt :: Int -> IPv4
fromInt i = toIPv4 $ map (\s -> (i `shiftR` s) .&. 0xff) [24, 16, 8, 0]

addressesInNetwork :: AddrRange IPv4 -> [IPv4]
addressesInNetwork i = takeWhile (`isMatchedTo` i) adds
    where net = toInt $ addr i
          adds = map (\s -> fromInt $ net + s) [0..]

successor :: Int -> IPv4 -> Maybe IPv4
successor r a = S.headMay $ dropWhile ( Maybe IPv4
net24 = successor 24

main::IO()
main = print $ parse "192.168.1.201" >>= net24


What I don't like: it just looks terrible. I look on it and see Perl, not Haskell.

Have I done something idiomatically wrong or my sense of beauty is broken?

Solution

I think what you've missed is the Enum instance for the IPv4 datatype from iproute. Using methods from Enum and Bounded you can eliminate the use of Data.Bits in your code.

Because IPv4 is Enum, your toInt method is just fromEnum :: (Enum a) => a -> Int. And similarly your fromInt is toEnum.

addressesInNetwork is largely the same logic, you just let someone else do all the work.

addressesInNetwork :: AddrRange IPv4 -> [IPv4]
addressesInNetwork range = takeWhile (`isMatchedTo` range) [low..]
    where
        (low, _) = addrRangePair range


I would use addrRangePair instead of the addr field accessor for two reasons. First, addr doesn't show up in the Haddock documentation for Data.IP which is a bit confusing. Second, on the off-chance that the author of iproute begins hiding the constructors of AddrRange from export (which is very common practice in Haskell so that library authors can change implementations without breaking users' code) you'll be safe from breaking changes.

Knowing the Enum instance and available Data.IP functions, you can now write a clearer version of successor. Unfortunately we have to be a bit cautious of bounds, Enum throws an exception if you try to get the successor of maxBound, but it's not too bad.

successor :: AddrRange IPv4 -> IPv4 -> Maybe IPv4
successor range ip | ip == maxBound              = Nothing
                   | succ ip `isMatchedTo` range = Just $ succ ip
                   | otherwise                   = Nothing


Notice I changed the type signature of your function to take an AddrRange instead of a mask length. This is kind of a separation of concerns issue, successor doesn't care about the mask length, what matters is whether an IP falls within the range it defines. So net24 would be implemented as—

net24 :: IPv4 -> Maybe IPv4
net24 ip = successor (makeAddrRange ip 24) ip


(That's kind of a strange name though, maybe successorMask24 instead?)

Code Snippets

addressesInNetwork :: AddrRange IPv4 -> [IPv4]
addressesInNetwork range = takeWhile (`isMatchedTo` range) [low..]
    where
        (low, _) = addrRangePair range
successor :: AddrRange IPv4 -> IPv4 -> Maybe IPv4
successor range ip | ip == maxBound              = Nothing
                   | succ ip `isMatchedTo` range = Just $ succ ip
                   | otherwise                   = Nothing
net24 :: IPv4 -> Maybe IPv4
net24 ip = successor (makeAddrRange ip 24) ip

Context

StackExchange Code Review Q#85846, answer score: 4

Revisions (0)

No revisions yet.