1
\$\begingroup\$

I have not yet developed a satisfying coding style in Haskell. In the code snippet below, there is a lot wrong to my taste:

  • Helper functions which should be local to showEngFloat are defined outside.
  • let ... in indentation madness
  • More than 80 columns

Can you show me how this same code can be written in a less indentation and pollution prone way?

I am aware that the name I chose for my function is not well chosen. (show... functions should return a ShowS type, right?)

import Numeric

normalize :: Int -> (Int,Int)
normalize dp =
    loop dp 0
    where
        loop dp e
            | dp > 3 = loop (dp - 3) (e+3)
            | dp < 1 = loop (dp + 3) (e-3)
            | otherwise = (dp,e)

toString :: [Int] -> [Char]            
toString ia = concat (fmap show ia)

padZeroes nz v  = v ++ take nz (repeat '0')

showEngFloat :: RealFloat a => Int -> a -> [Char]
showEngFloat precision x
    | x >= 0.0 = calc x
    | otherwise = "-" ++ calc (-x)
    where
        calc n =
            let (digits,dotPos0) = floatToDigits 10 n in
                let (ndigits,(dotPos1,e)) = (length digits,normalize dotPos0) in
                    let a
                            | ndigits >= dotPos1 = toString (take dotPos1 digits)
                            | otherwise = padZeroes (dotPos1 - ndigits) (toString (take ndigits digits))
                            in
                            let b = padZeroes (precision - max dotPos1 ndigits) (toString (drop dotPos1 digits))
                                in
                                    let c = toString [e] in
                                        a ++ "." ++ b ++ "E" ++ c
\$\endgroup\$
0

3 Answers 3

1
\$\begingroup\$

How about floating out subfunctions and factoring lets ?

calc :: RealFloat a => a -> Int -> [Char]
calc n precision =
  let (digits,dotPos0) = floatToDigits 10 n 
      (ndigits,(dotPos1,e)) = (length digits,normalize dotPos0)
      b = padZeroes (precision - max dotPos1 ndigits) (toString (drop dotPos1 digits))
      c = toString [e]
      a
        | ndigits >= dotPos1 = toString (take dotPos1 digits)
        | otherwise = padZeroes (dotPos1 - ndigits) (toString (take ndigits digits)) in
      a ++ "." ++ b ++ "E" ++ c

showEngFloat :: RealFloat a => Int -> a -> [Char]
showEngFloat precision x
    | x >= 0.0 = calc x precision
    | otherwise = "-" ++ calc (-x) precision

You can probably rewrite this further still.

\$\endgroup\$
4
  • \$\begingroup\$ Those extra functions are (in my eyes) implementation details of the main function. Moving them outside pollutes the namespace, I think. Probably suggests to potential users of the module that those implementation helpers actually are part of an "official" API... \$\endgroup\$
    – BitTickler
    Sep 12, 2016 at 14:56
  • 1
    \$\begingroup\$ Your module's header filters which functions it exports. This is where you avoid namespace pollution. \$\endgroup\$
    – V. Semeria
    Sep 12, 2016 at 14:58
  • \$\begingroup\$ Why do a and b need extra lets and the second line in first let does not? \$\endgroup\$
    – BitTickler
    Sep 12, 2016 at 15:00
  • \$\begingroup\$ They actually don't. Look at the updated answer. \$\endgroup\$
    – V. Semeria
    Sep 12, 2016 at 15:04
3
\$\begingroup\$

Here's another way to write it (without changing too much). The functions toString and padZereos do sensible things on their own, so I kept them in the global namespace. Everything else is local. Having one big where-clause where you define everything local to a function is something you'll see often. Note that due to lazy evaluation you can define a_trimmed and a_padded, even if only one is used, so the other is never calculated. That further cleans up the code.

In a next step, I'd probably get rid of calc entirely, and fold sign handling into its body. Also, normalize looks like it should be calculated with divMod or quotRem.

showEngFloat :: RealFloat a => Int -> a -> [Char]
showEngFloat precision x = result where
  result = if x >= 0 then calc x else '-' : calc (-x)
  normalize :: Int -> (Int,Int)
  normalize dp = loop dp 0 where
    loop dp e
      | dp > 3 = loop (dp - 3) (e+3)
      | dp < 1 = loop (dp + 3) (e-3)
      | otherwise = (dp,e)
  calc n = a ++ "." ++ b ++ "E" ++ c where
    (digits,dotPos0) = floatToDigits 10 n
    ndigits = length digits
    (dotPos1,e) = normalize dotPos0
    a_trimmed = toString (take dotPos1 digits)
    a_padded  = padZeroes (dotPos1 - ndigits) (toString (take ndigits digits))
    a = if ndigits >= dotPos1 then a_trimmed else a_padded
    b = padZeroes (precision - max dotPos1 ndigits) (toString (drop dotPos1 digits))
    c = toString [e]
\$\endgroup\$
1
  • \$\begingroup\$ This is about the version I had in mind when I started. Then I ran into troubles and ended with the terrible stuff I showed :) Thanks! \$\endgroup\$
    – BitTickler
    Sep 12, 2016 at 15:50
0
\$\begingroup\$

I'll hijack dirkt's solution and add my two cents.

showEngFloat :: RealFloat a => Int -> a -> [Char]
showEngFloat precision x
  =  if x >= 0 then "" else "-"
  ++ padZeroes dotPos1 digitstring
  ++ "."
  ++ padZeroes precision (drop dotPos1 digitstring)
  ++ "E"
  ++ show (e * 3) where
  (digits,dotPos0) = floatToDigits 10 (abs x)
  digitstring = foldMap show digits
  (e, dotPos1) = divMod dotPos0 3

padZeroes l v = take l (v ++ repeat '0')

(Not quite sure plugging precision into padZeroes directly is right.)

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

Not the answer you're looking for? Browse other questions tagged or ask your own question.