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!).

Answer 2

I see there is already an accepted answer, but just giving another solution (although very similar). Following the guidelines outlined here: https://www.fpcomplete.com/blog/2016/11/exceptions-best-practices-haskell (a good thing to read either way), you would get something like this.

import Control.Monad.Catch

data LatitudeException = LatitudeException
instance Show LatitudeException where
  show LatitudeException = "Invalid Latitude"
instance Exception LatitudeException

data LongitudeException = LongitudeException
instance Show LongitudeException where
  show LongitudeException = "Invalid Longitude"
instance Exception LongitudeException

mkLatLngPoint :: (MonadThrow m) => LatitudeDMS -> LongitudeDMS -> Datum -> m LatLng
mkLatLngPoint lat lng dtm = do
  lt <- evalLatitude lat
  ln <- evalLongitude lng
  let p = LatLngPoint { latitude = lt , longitude = ln, height = 0 }
  return $ LatLng { point = p , datum = dtm }

  where evalLatitude :: (MonadThrow m) => LatitudeDMS -> m Double
        evalLatitude (North p) = case dmsToLatLngPoint p 1 of
                                  (Just d) -> return d
                                  Nothing -> throwM LatitudeException
        evalLatitude (South p) = case dmsToLatLngPoint p (-1) of
                                  (Just d) -> return d
                                  Nothing -> throwM LatitudeException

        evalLongitude :: (MonadThrow m) => LongitudeDMS -> m Double
        evalLongitude (East p) = case dmsToLatLngPoint p 1 of
                                  (Just d) -> return d
                                  Nothing -> throwM LongitudeException
        evalLongitude (West p) = case dmsToLatLngPoint p (-1) of
                                  (Just d) -> return d
                                  Nothing -> throwM LongitudeException

        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))

There is definitely a little more boilerplate to deal with but gives a little more flexibility. Check out the article and see if the benefits outway that for your situation.