String Calculator kata in Haskell In order to practice some Haskell, I tried this kata, where the task is to add some numeric inputs that are delimited by commas or newlines.

I have the below solution in Haskell which passes my tests, but could anyone offer any improvements? It feels a bit messy to me so feedback on how to make it more functional (my experience is pretty much all OO until now) would be great!

Known failings:

This does not try to be smart about invalid syntax so the pattern matching is not exhaustive - would the best solution here be to use a try/either type? This seemed like it would add quite a bit of noise at this stage so for now I have left this. It also does not fail on negative numbers for the same reason.

Code:

module Calculator where

import Data.List.Split

defaultDelimiters = [",", "\n"]

readInt :: String -&gt; Int
readInt = read

splitAtDelims :: ([String], String) -&gt; [String]
splitAtDelims (delims, body) = foldr splitEachOnSubStr [body] delims
    where 
        splitOnSubStr = split . dropBlanks . dropDelims . onSublist
        splitEachOnSubStr = concatMap . splitOnSubStr

parseDelim :: String -&gt; String -&gt; (String, String)
parseDelim (']':xs) delim = (delim, xs)
parseDelim (x:xs) delim = parseDelim xs (delim++[x])

parseCustomDelimiters :: [String] -&gt; String -&gt; ([String], String)
parseCustomDelimiters [] (delim:'\n':body) = ([[delim]], body)
parseCustomDelimiters delims ('\n':rest)   = (delims, rest)
parseCustomDelimiters delims ('[':rest)    = parseCustomDelimiters newDelims remainingText
    where
        parsed = parseDelim rest ""
        newDelims = delims ++ [fst parsed]
        remainingText = snd parsed

parse :: String -&gt; ([String], String)
parse ('/':'/':text)    = parseCustomDelimiters [] text
parse body              = (defaultDelimiters, body)

add :: String -&gt; Int
add = sum . filter (&lt; 1000) . map readInt . splitAtDelims . parse
I'd suggest to accompany the code with comments. Not just for the purpose of review, but also for your own. After a few weeks it's hard to remember all the details, and what seems to be clear now will be hard to read later. Especially in this case when all arguments are of String or [String], it'd be very helpful to document them (see Function arguments).

Some more thoughts: The code isn't that easy to read. One reason for that I see
is that your functions often entangle together several goals. Extracting common
functionality into smaller helper functions would help.

Specific comments:

parse :: String -&gt; ([String], String)
parse ('/':'/':text)    = parseCustomDelimiters [] text
parse body              = (defaultDelimiters, body)


The name is basically meaningless - what does it parse? Why do you call the
string text in one place and body in another?

parseDelim :: String -&gt; String -&gt; (String, String)
parseDelim (']':xs) delim = (delim, xs)
parseDelim (x:xs) delim = parseDelim xs (delim++[x])


This basically is "read until a specific character and return both parts". And
the second argument is redundant, it's only an implementation detail, and
obscures how the function should be used.  I'd create a helper function like
readUntil :: Char -&gt; String -&gt; (String, String), and implement it using
break.
And then just parseDelim = readUntil ']'.

splitAtDelims :: ([String], String) -&gt; [String]
splitAtDelims (delims, body) = foldr splitEachOnSubStr [body] delims
    where 
        splitOnSubStr = split . dropBlanks . dropDelims . onSublist
        splitEachOnSubStr = concatMap . splitOnSubStr


The type is quite uncommon. I'd rather use

splitAtDelims :: [String] -&gt; String -&gt; [String]


and then use uncurry splitAtDelims when needed.

parseCustomDelimiters :: [String] -&gt; String -&gt; ([String], String)
parseCustomDelimiters [] (delim:'\n':body) = ([[delim]], body)
parseCustomDelimiters delims ('\n':rest)   = (delims, rest)
parseCustomDelimiters delims ('[':rest)    = parseCustomDelimiters newDelims remainingText
    where
        parsed = parseDelim rest ""
        newDelims = delims ++ [fst parsed]
        remainingText = snd parsed


Again, the first argument is just an implementation detail and makes the
function both harder to read and harder to use. Rather you should define it for
example as:

parseCustomDelimiters :: String -&gt; ([String], String)
parseCustomDelimiters = loop []
  where
    loop =  -- your original definition


Also you use both names body and rest for the same purpose, which is very
confusing. You could also use readUntil from before to simplify this, as
apparently new-line terminates processing the input. So you could just split the
body on '\n' and parse the delimiters from the first part.

Notice that when parsing delimiters, you match on the starting marker "//" in
a different function than the ending marker "\n". It's much easier to read
when such related parts are together, in one function. So you could write
another helper function, like

readBetween :: String -&gt; String -&gt; (String, String)


where the first and the second argument are the starting and ending makers. Then
you could write readBetween "//" "\n" text and parse the returned content.



I'd strongly encourage you to explore ReadP, which is available in base, or some other parser (like Parsec AttoParsec). The code will be then much more readable!

You can also go without such a full-featured parser, and instead implement your own, tiny one. Already you can see in your types that the core structure of a parsing function (that doesn't handle errors or multiple possibilities) is

String -&gt; (a, String)


That is, we parse some expected value of type a and return the rest of the string. A parser is then just an encapsulation of such an idea, like newtype Parser a = Parser (String -&gt; (a, String)) and common operations on them.



As mentioned in the comments, you didn't specify any tests, even though this was a TDD exercise. Including them would be definitely helpful. And a great way to learn about property-based testing! Testing is also extremely valuable for refactorings, to make sure you don't introduce a regression.

I hope this helps!