6
\$\begingroup\$

I'm an advanced C# programmer learning F#. As an exercise I'm porting a function that calculates the check digit of a US ABA (routing) number. Here are 2 C# implementations:

int CalcCheckDigit(string rt)
{
    var s = new[]{3,7,1,3,7,1,3,7}
        .Zip(rt, (m,d) => m * int.Parse(d.ToString()))
        .Sum();

    return (int)Math.Ceiling(s / 10.0) * 10 - s;
}

int CalcCheckDigitOldSchool(string rt)
{
    int[] mults = new[]{3,7,1,3,7,1,3,7};
    int s = 0;
    for (int i = 0; i < mults.Length; i++)
    {
        int digit = int.Parse(rt[i].ToString());
        s += digit * mults[i];
    }

    double nextMultOfTen = Math.Ceiling(s / 10.0) * 10;
    return (int)nextMultOfTen - s;
}

And here's my crack at it with F#:

let calcCheckDigit rt =
    let s = 
        rt
        |> Seq.zip [3;7;1;3;7;1;3;7]
        |> Seq.map (fun (a,b) -> a * int(string b))
        |> Seq.sum |> float
    int (ceil (s / 10.0) * 10.0 - s)

How might the F# version be improved?

\$\endgroup\$
7
\$\begingroup\$

This isn't really F# specific because you could do the same thing in your C# code, but converting each character to a string so you can parse it as an integer seems inefficient to me. I'd be inclined to use Char.GetNumericValue instead.

Also, you could eliminate a step in your method chain by using Seq.sumBy instead of Seq.map + Seq.sum.

I might write your code like this:

open System

let calcCheckDigit rt =
    let s = 
        rt
        |> Seq.map (Char.GetNumericValue >> int)
        |> Seq.zip [3;7;1;3;7;1;3;7]
        |> Seq.sumBy (fun (a,b) -> a * b)
        |> float
    (s / 10.0 |> ceil) * 10.0 - s |> int

In my opinion, separating out the "convert a character to an integer" from the multiplication makes it easier to read. I'm using the function-composition operator to join together the Char.GetNumericValue and int functions in sequence, and then passing the resulting combined function into Seq.map.

On the last line, it's a little ambiguous just what value is being passed into ceil. It would be easy for someone unfamiliar with the code to assume that the result of the entire expression (s / 10.0) * 10.0 - s is being passed to ceil. I would be inclined to write that as I did above (obviously) but if you don't like that use of the pipe operator then I think this would also be a little more clear: int ((ceil (s / 10.0)) * 10.0 - s). Personally I'm not a fan of lots of nested parens, but to each their own.

(Before I edited this post, I mistakenly assumed exactly what I just mentioned, and so I wrote (s / 10.0) * 10.0 - s |> ceil |> int. Then I realized it wouldn't produce the same output as the C# version.)

\$\endgroup\$
  • \$\begingroup\$ I would be careful about char.GetNumericValue(), because it accepts characters like ½ or Ⅻ. \$\endgroup\$ – svick Jul 12 '13 at 0:40
  • \$\begingroup\$ Well I would hope that before one calls a function that calculates the check digit of a US ABA (routing) number, one would validate that the user input is all integers... \$\endgroup\$ – Joel Mueller Jul 12 '13 at 5:06
3
\$\begingroup\$

I like Joel Mueller's answer the best, but here is my alternative solution that is similar to your second C# sample and would probably be more efficient than map*zip*sum:

let calcCheckDigit (rt : string) =
  let lookup = [| 3.;7.;1.;3.;7.;1.; 3.; 7. |]
  let s = Array.sum [| for i in 0 .. rt.Length-1 -> lookup.[i] * (float << string) rt.[i] |]
  int ((ceil (s / 10.0)) * 10.0 - s)
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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