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

String calculator in F#

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

Problem

This is some of my first real F# code ever. I have done a bit of reading and watched a few videos however. I chose to do a code kata for string calculator to try it out.

The kata I was working on is here by Roy Osherove, though I may have strayed a bit.

I am pretty happy with this, but wonder if there are things that could be better, or more idiomatic F#. I am specifically wondering if there is a better option to having the overloaded Add members. I'm also curious about the last test and exception handling best practices.

module Tests
open Xunit
open System

type Calculator() =
    let delimiters = ",\n"
    member x.Add (m:int, n:int list) =
        match n with
        |[] -> m
        |y::ys -> 
            if y  x.ToString() |> Convert.ToInt32) (delimiters.ToCharArray() |> y.Split |> Seq.toList)
        x.Add(0,numList)

[]
let ReturnsNotNull() = 
    let calc = new Calculator()
    Assert.NotNull (calc.Add "0,0")

[]
let ReturnsZeroWhenZeros() = 
    let calc = new Calculator()
    Assert.Equal(0,(calc.Add "0,0"))

[]
let ReturnsOneWhenShouldBeOneOnLeft() = 
    let calc = new Calculator()
    Assert.Equal(1,(calc.Add "1,0"))
[]
let ReturnsOneWhenShouldBeOneOnRight() = 
    let calc = new Calculator()
    Assert.Equal(1,(calc.Add "0,1"))
[]
let ReturnsElevenWithStringOfNumbersThatTotalEleven() = 
    let calc = new Calculator()
    Assert.Equal(11,(calc.Add "0,1,1,1,1,1,6"))
[]
let ReturnsElevenWithStringOfNumbersThatTotalElevenDelimitedByNewLine() = 
    let calc = new Calculator()
    Assert.Equal(11,(calc.Add "0,1,1\n1,2\n6"))
[]
let ReturnsElevenWithStringOfNumbersThatTotalElevenDelimitedByNewLineNoNegativeNumbers() = 
    let calc = new Calculator()
    try
        calc.Add("0,1,-1\n1,2\n6") |> ignore
        Assert.False true
    with
        | _ -> Assert.True true

Solution

Is there a specific reason you want to accept a string? You could pass a list and get by with one overload.

type Calculator() =
  member x.Add(nums: int list) = List.sum nums

let calc = Calculator()

calc.Add([0; 0])
calc.Add([1; 2; 3; 4])
calc.Add(List.init 10 id)


EDIT

I'm sorry, I misunderstood the point of the exercise. There are a few changes I would make.

  • According to the kata, you only need one method taking a string and returning an int.



  • There's an overload of String.Split accepting a char array, so you can store your delimiters as such and avoid the call to ToCharArray.



  • You can use the built-in int function instead of Convert.ToInt32.



  • There's no need to roll your own "sum" function. It's already built into the various collection modules.



The following code satisfies steps 1-3.

type Calculator() =

  let delimiters = [|','; '\n'|]

  member x.Add(nums) = 
    match nums with
    | null -> nullArg "nums"
    | "" -> 0
    | _ -> 
      nums.Split(delimiters) 
      |> Array.map int 
      |> Array.sum

Code Snippets

type Calculator() =
  member x.Add(nums: int list) = List.sum nums

let calc = Calculator()

calc.Add([0; 0])
calc.Add([1; 2; 3; 4])
calc.Add(List.init 10 id)
type Calculator() =

  let delimiters = [|','; '\n'|]

  member x.Add(nums) = 
    match nums with
    | null -> nullArg "nums"
    | "" -> 0
    | _ -> 
      nums.Split(delimiters) 
      |> Array.map int 
      |> Array.sum

Context

StackExchange Code Review Q#10382, answer score: 3

Revisions (0)

No revisions yet.