HiveBrain v1.2.0
Get Started
← Back to all entries
patternMajor

Haskell functions to extract and reverse integers hiding in strings

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
reversehidinghaskellextractandfunctionsintegersstrings

Problem

There may not be a "right" way to do it, but I'm inexperienced in functional programming. I'd like some feedback on how the code can be written in a more idiomatic way.

Here it is:

import Data.Maybe (fromJust, isJust)

sepInt n = if n >= 10
            then  ( sepInt ( n `div` 10 ) ) ++ ((n `mod` 10):[])
            else n `mod` 10 : []

getStuff (x:xs) = if isJust x || null xs
                    then fromJust x
                    else getStuff xs

base10IntTOstring num =
                 let chars = [ (1, '1'), (2, '2'), (3, '3'), (4, '4'), (5, '5'), (6, '6'), (7, '7'), (8, '8'), (9, '9'), (0, '0') ]
                     numbahs = sepInt num in
                     map getStuff ( map (\a -> (map (\(x,y) -> if a == x then Just y else Nothing ) chars ) ) numbahs )

charTOBase10Int char =
                 let chars = [ ('1', 1), ('2', 2), ('3', 3), ('4', 4), ('5', 5), ('6', 6), ('7', 7), ('8', 8), ('9', 9), ('0', 0) ] in
                     let charslist = ( map (\(a,x) -> if char == a then Just x else Nothing) chars ) in
                         getStuff charslist

stringTOBase10Int string =
    let integers = ( map charTOBase10Int string ) in
        let multByBase i (x:xs) = if null xs
                                   then (10^i)*x
                                   else (10^i)*x + multByBase ( i-1 ) xs
        in multByBase ( (length integers)-1 ) integers


It also reverses integers.

sepInt n = if n >= 10
            then  ( sepInt ( n `div` 10 ) ) ++ ((n `mod` 10):[])
            else n `mod` 10 : []

reverseInteger string =
    let integers = sepInt string in
        let multByBase i (x:xs) = if null xs
                                   then (10^i)*x
                                   else (10^i)*x + multByBase ( i-1 ) xs
        in multByBase ( (length integers) - 1 ) integers

Solution

Let's take one function at a time, until we're all done.

sepInt

sepInt n = if n >= 10
            then  ( sepInt ( n `div` 10 ) ) ++ ((n `mod` 10):[])
            else n `mod` 10 : []


First things first: you'll definitely want to learn a bit about precedence! Normally I'm in favor of adding some unnecessary parentheses if it helps disambiguate a strange situation or if the operators involved aren't often mixed, but too many of them can get in the way of readability. Also, take advantage of that sweet syntactic sugar for lists that the language provides! So iteration one of this function is

sepInt n = if n >= 10
            then sepInt (n `div` 10) ++ [n `mod` 10]
            else [n `mod` 10]


Now, as we all know, building up a linked list by repeatedly appending to the end is a bit inefficient. Probably for such small lists as you'll be using in test cases here it won't matter, but it's a good idea to get in the habit of paying attention to some of the easiest stuff, so let's try to improve this a bit. We have a choice here: either we can keep the interface of this function as-is, that is, always output a list in the right order, or we can choose to change the interface, and change all the call-sites of this function. I think for this case we can keep the interface. The idea we'll take is to build up the list backwards, then reverse it at the very end. The name go is traditional for local workers.

sepInt = reverse . go where
    go n = if n >= 10
            then [n `mod` 10] ++ go (n `div` 10)
            else [n `mod` 10]


There's something a bit funny about this base case to me. It seems like it's not the most basic one you could choose. If we let the "loop" run one more time...

sepInt = reverse . go where
    go n = if n > 0
            then [n `mod` 10] ++ go (n `div` 10)
            else []


There's a few things I find more satisfying about this: our base-case input is 0, a common base for Integers; our base-case output is [], a common base for []s; and there's no duplicated code in the two branches of the if. Finally, I think I'd choose to replace the if-then-else with a pattern match, noting however that this function has a slightly different behavior for negative numbers. Since we were never really doing the right thing for negative numbers, this doesn't bother me too much.

sepInt = reverse . go where
    go 0 = []
    go n = [n `mod` 10] ++ go (n `div` 10)


If we're feeling fancy, we can choose to use divMod instead of two separate calls to div and mod; and we can unroll the definition of (++); but I think neither of these is terribly important. Nevertheless, they're idiomatic, so:

sepInt = reverse . go where
    go 0 = []
    go n = let (d, m) = n `divMod` 10 in m : go d


Okay, let's check our work. We already know that the final thing works differently for negative numbers, so let's only check non-negative ones.

*Main Test.QuickCheck> quickCheck (\(NonNegative n) -> sepInt n == sepInt' n)
*** Failed! Falsifiable (after 2 tests):  
NonNegative {getNonNegative = 0}


Whoa, whoops! Can you figure out which refactoring above was the culprit? =)

Now we have to decide whether we like the old behavior better or the new one. I think in this particular case we should like the old behavior better, since the goal is to show a number, and we'd like 0 to show up as "0" rather than as "". It's a bit ugly, but we can special-case it. Since we like our future selves, we'll leave ourselves a note about this, too.

-- special case for a human-readable 0
sepInt 0 = [0]
sepInt n = reverse . go $ n where
    go 0 = []
    go n = let (d, m) = n `divMod` 10 in m : go d


Now the test passes:

*Main Test.QuickCheck> quickCheck (\(NonNegative n) -> sepInt n == sepInt' n)
+++ OK, passed 100 tests.


getStuff

getStuff (x:xs) = if isJust x || null xs
                    then fromJust x
                    else getStuff xs


This name sure leaves something to be desired! And it leaves another important thing to be desired, too: there's lots of inputs where it just crashes. Nasty! It turns out that you never call it on inputs of that form later, but totality is another good habit that you should get yourself into. It's just another tool in the mature programmer's defensive programming toolbelt. In our case, we'll want to handle cases like [], or [Nothing], or [Nothing, Nothing], etc. where there's no good answer to return. What should we return if that happens?

One simple and quite common choice is to change our type from

getStuff :: [Maybe a] -> a


to

getStuff :: [Maybe a] -> Maybe a


but I think that's a bit short-sighted. Ignoring for the moment the inputs we know we're going to call this thing on, we've observed already that there's times when there's no good answer to return, and there's times when there is a good answer to return, so Maybe a seems like a good start, but ther

Code Snippets

sepInt n = if n >= 10
            then  ( sepInt ( n `div` 10 ) ) ++ ((n `mod` 10):[])
            else n `mod` 10 : []
sepInt n = if n >= 10
            then sepInt (n `div` 10) ++ [n `mod` 10]
            else [n `mod` 10]
sepInt = reverse . go where
    go n = if n >= 10
            then [n `mod` 10] ++ go (n `div` 10)
            else [n `mod` 10]
sepInt = reverse . go where
    go n = if n > 0
            then [n `mod` 10] ++ go (n `div` 10)
            else []
sepInt = reverse . go where
    go 0 = []
    go n = [n `mod` 10] ++ go (n `div` 10)

Context

StackExchange Code Review Q#24782, answer score: 26

Revisions (0)

No revisions yet.