7
\$\begingroup\$

Question

How can the following implementation of fractionParse be made less ugly? (Preserving unreduced numerators and denominators is supposed to be a feature---as shown in the second example---so using readMaybe s :: Maybe (Ratio Int) doesn't really help.)

Things that irked me in the process of writing it, include:

  • Not being able to find a way to use do-notation in conjunction with Maybe in a way that would return as soon as Just appears rather than when the first Nothing appears. In other words a Maybe monad instance that emulates or rather than and.
  • case and pattern matching colliding in unhelpful ways, forcing me to write nested if-then-elses.

Usage examples

λ> fractionParse "2/3"
Fraction 2 3
λ> fractionParse "12 / 6"
Fraction 12 6
λ> fractionParse "23"
Whole 23
λ> fractionParse "a"
Rubbish
λ> fractionParse "2/3/4"
Rubbish
λ> fractionParse "2%3"
Rubbish

Implementation

import Text.Read (readMaybe)
import Data.String.Utils (split)
import Data.Maybe (isJust, fromJust)

data FractionParse = Fraction Int Int
                   | Whole Int
                   | Rubbish deriving (Show, Eq)

fractionParse :: String -> FractionParse
fractionParse s = let i = maybeInt s
                      r = maybeNumDenom s in
  if isJust r
  then let (Just (n,d)) = r in Fraction n d
  else if isJust i
       then Whole (fromJust i)
       else Rubbish

maybeInt :: String -> Maybe Int
maybeInt s = readMaybe s

maybeNumDenom :: String -> Maybe (Int, Int)
maybeNumDenom s = do
  if containsExactlyOneSlash s then Just () else Nothing
  let [ns,ds] = split "/" s
  n <- readMaybe ns
  d <- readMaybe ds
  return (n,d)

containsExactlyOneSlash s = (length $ filter (=='/') s) == 1
\$\endgroup\$
  • \$\begingroup\$ As we all want to make our code more efficient or improve it in one way or another, try to write a title that summarizes what your code does, not what you want to get out of a review. Please see How to get the best value out of Code Review - Asking Questions for guidance on writing good question titles. \$\endgroup\$ – Mast Jun 28 '16 at 17:07
2
\$\begingroup\$

You're looking for the First Monoid instance for Maybe. Used like—

import Data.Foldable (foldMap)
import Data.Monoid (First(..))

fractionParse :: String -> FractionParse
fractionParse s = fromMaybe Rubbish . getFirst
                $ foldMap First [ maybeFraction s
                                , maybeWhole s
                                ]

It doesn't make sense to encode your own failure values in your datatypes, so I'd remove the Rubbish Constructor.

data FractionParse = Fraction Int Int | Whole Int
  deriving (Show, Eq)

fractionParse :: String -> Maybe FractionParse

Use Control.Monad.guard instead of manually sending up your own sentinel values (as in maybeNumDenom).

maybeNumDenom s = do
  guard $ containsExactlyOneSlash s
  -- ...

Take advantage of incremental parsing to implement maybeNumDenom, then you won't have to do so much extraneous filtering, counting, and finger crossing. And by removing the Rubbish constructor, at the point you have a correct parse you know you can return a Fraction, so—

import Data.Maybe (listToMaybe)

maybeFraction :: String -> Maybe FractionParse
maybeFraction s = listToMaybe $ do
  (n, '/':s') <- reads s
  (d, "") <- reads s'
  return (Fraction n d)

The above function operates in the list monad. listToMaybe converts a list to a Maybe value by returning Just the first element of the list, or Nothing in the case of an empty list. reads :: Read a => String -> [(a, String)] produces possible parses from a given string, returning the remainder of the unparsed string as the second element of each tuple. Binding to (d, "") ensures that only parses that consume the whole string will be returned.

\$\endgroup\$
  • \$\begingroup\$ Lots of good stuff there, thanks! The original fractionParse was a lot more tolerant WRT whitespace than maybeFraction. For now I'm struggling to recover that tolerance. \$\endgroup\$ – jacg Jun 28 '16 at 19:37
  • \$\begingroup\$ @jacg Oops! Fixed. \$\endgroup\$ – R B Jun 28 '16 at 20:38
  • \$\begingroup\$ It could probably do with import Data.Maybe (fromMaybe) too, for those who want to copy 'n' paste. \$\endgroup\$ – jacg Jun 28 '16 at 20:48
  • 1
    \$\begingroup\$ The Alternative instance of Maybe also has First behavior. fractionParse s = fromMaybe Rubbish $ maybeFraction s <|> maybeWhole s \$\endgroup\$ – Gurkenglas Jun 30 '16 at 15:22

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.