patterncsharpMinor
Learning F# - Porting C# Function to F#
Viewed 0 times
learningportingfunction
Problem
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:
And here's my crack at it with F#:
How might the F# version be improved?
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?
Solution
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:
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
On the last line, it's a little ambiguous just what value is being passed into
(Before I edited this post, I mistakenly assumed exactly what I just mentioned, and so I wrote
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 |> intIn 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.)Code Snippets
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 |> intContext
StackExchange Code Review Q#28378, answer score: 7
Revisions (0)
No revisions yet.