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 -> ([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 -> String -> (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 -> String -> (String, String)
, and implement it using
break
.
And then just parseDelim = readUntil ']'
.
splitAtDelims :: ([String], String) -> [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] -> String -> [String]
and then use uncurry splitAtDelims
when needed.
parseCustomDelimiters :: [String] -> String -> ([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 -> ([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 -> String -> (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 -> (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 -> (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!