haskell/nucleotide-count: Add mentoring notes

[sshine]

No description provided.

[frerich]

Looks good! Another solution I'd like to propose is based on Map.fromListWith (+):

nucleotideCounts :: String -> Either String (Map Char Int)
nucleotideCounts xs
    | all (`elem` "ACGT") xs = Right (counts `Map.union` defaults)
    | otherwise              = Left xs
  where
    counts   = Map.fromListWith (+) (zip xs [1,1..])
    defaults = Map.fromList (zip "ACGT" [0,0..])

The summing part is hididen in Map.fromListWith (+) here. Since it's possible that a certain nucleotide does not occur even once, we define a map of 'default' counts (i.e. all zero) which is then used to form the final map -- the order of arguments to Map.union matters here!

It could be argued that doing two passes over the input list (one by the all call in the guard, the other by evaluating the counts definition) is inefficient, but I like it for being quite clear about when this function yields a Right value and when Left.

[sshine]

Very nice, @frerich. Thanks! Since exercism/haskell#714 we're using a custom data type

data Nucleotide = A | C | G | T deriving (Eq, Ord, Show)

I'll try and adapt your solution to this data type and add this.

[frerich]

I imagine replacing "ACGT" with [A,C,G,T] (or maybe deriving Enum and then using [A .. T] if you fancy) ought to be enough. :-)

[sshine]

@frerich: I like the use of M.fromListWith (+), so I merged it with my previous favorite solution and added the use of readEither and <> for error conversion, so it now relies entirely on the Read instance (and there are no separate validation functions that can desync from the data type). I also added Enum and Bounded so that the zero map can be generated as

M.fromListWith (+) [minBound..] [0,0..]

[sshine]

OK, I think I've golfed this enough.

I had another idea of

{-# LANGUAGE TypeApplications #-}

isNucleotide :: Char -> Bool
isNucleotide = isJust . readMaybe @Nucleotide . return

but I don't think there are any of the current reasonable solutions that benefit from this, since the ones that use isNucleotide subsequently use read, so they might as well have used readMaybe or readEither to begin with.

[sshine]

@frerich: Could you re-approve my changes?

[frerich]

Looks good to me, except for one single sentence which seems somewhat dubious to me.

[frerich]

This sentence seems a little fishy to me -- four separate length . filter (== c) calls "do not necessarily" perform bad?

It's true that GHC can fuse list combinators, but to the best of my knowledge this only works if you compose certain functions (at least catamorphisms). I'm not sure this claim is accurate when applied to four independent invocations of count.

[sshine]

I'll have to run a benchmark then.

Sounds like you're right.

I recall this one not running nearly like four times for large input.

[sshine]

This was still in my queue of things to address once I got home from vacation (yesterday), but you've only bumped it up the queue, since the mentor notes are now somehow incorrect. :-D

I'll fix this Monday.