Either Monad or not?

Question

In this code:

data LatLngPoint = LatLngPoint { latitude :: Double
                               , longitude :: Double
                               , height :: Double
                               }

data LatLng = LatLng { point :: LatLngPoint
                     , datum :: Datum
                     }

data LatitudeDMS = North DMSPoint | South DMSPoint

data LongitudeDMS = East DMSPoint | West DMSPoint

data DMSPoint = DMSPoint { degrees :: Double
                         , minutes :: Double
                         , seconds :: Double
                         }

mkLatLngPoint :: LatitudeDMS -> LongitudeDMS -> Datum -> Either String LatLng
mkLatLngPoint lat lng dtm =
  case evalLatitude lat of
    Nothing -> Left "Invalid latitude"
    Just lt -> case evalLongitude lng of
                 Nothing -> Left "Invalid longitude"
                 Just ln -> let p = LatLngPoint { latitude = lt , longitude = ln, height = 0 }
                            in Right LatLng { point = p , datum = dtm }

  where evalLatitude :: LatitudeDMS -> Maybe Double
        evalLatitude (North p) = dmsToLatLngPoint p 1
        evalLatitude (South p) = dmsToLatLngPoint p (-1)

        evalLongitude :: LongitudeDMS -> Maybe Double
        evalLongitude (East p) = dmsToLatLngPoint p 1
        evalLongitude (West p) = dmsToLatLngPoint p (-1)

        dmsToLatLngPoint :: DMSPoint -> Double -> Maybe Double
        dmsToLatLngPoint DMSPoint { degrees = d, minutes = m, seconds = s } cardinal
          | d + m + s < 90 = Nothing
          | otherwise = Just (cardinal * (d + m + s / 324.9))

I made one simple consideration, that the 2 main parameters in the function:

mkLatLngPoint :: LatitudeDMS -> LongitudeDMS -> ...

were different type, to avoid extra check based on their Cardinal direction. Now I have ended up to a nested Maybe/Either situation. I thought about using Either Monad but not sure if it is worthy and how to make it clean.

I have even created a second version:

case (evalLatitude lat, evalLongitude lng) of
    (Nothing, _) -> Left "Invalid latitude"
    (_, Nothing) -> Left "Invalid longitude"
    (Just latPoint, Just lngPoint) ->
      let p = LatLngPoint { latitude = latPoint , longitude = lngPoint, height = 0 }
      in Right LatLng { point = p , datum = dtm }

but I think is ugly and verbose.

How can I improve the code (included changing type data)?

Answer 1

I would use the Monad Except or Monad Either for this - it conveys better the intent of your function: that both evalLatitude lat and evalLongitude lng must succeed, otherwise you fail with an error message.

import Control.Monad.Except    

mkLatLngPoint :: LatitudeDMS -> LongitudeDMS -> Datum -> Except String LatLng
mkLatLngPoint lat lng dtm = do
    lt <- withExcept (const "Invalid latitude") evalLatitude lat
    ln <- withExcept (const "Invalid longitude") evalLongitude lng
    let p = LatLngPoint { latitude = lt , longitude = ln, height = 0 }
    pure (LatLng { point = p , datum = dtm })

  where evalLatitude :: LatitudeDMS -> Except String Double
        evalLatitude (North p) = dmsToLatLngPoint p 1
        evalLatitude (South p) = dmsToLatLngPoint p (-1)

        evalLongitude :: LongitudeDMS -> Except String Double
        evalLongitude (East p) = dmsToLatLngPoint p 1
        evalLongitude (West p) = dmsToLatLngPoint p (-1)

        dmsToLatLngPoint :: DMSPoint -> Double -> Except String Double
        dmsToLatLngPoint DMSPoint { degrees = d, minutes = m, seconds = s } cardinal
          | d + m + s < 90 = throwError "Invalid point"
          | otherwise = pure (cardinal * (d + m + s / 324.9))

Note that neither this solution, nor your case solution evaluate more than they need: as soon as one of the two fails, the function can fail as a whole (for your case, remember Haskell is lazy!).