4
\$\begingroup\$

the TotalSavings() function on the record below has a code smell that I can't exactly detail. Is there a more readable alternative for implementing this function on a record type?

type ProvidersCharged = ProvidersCharged of decimal
type InsuranceSavings = InsuranceSavings of decimal

type ClaimsSummary = 
    { Claims:Claim seq
      ProvidersCharged:ProvidersCharged
      InsuranceSavings:InsuranceSavings 
    } member this.TotalSavings() = match this.ProvidersCharged with 
                                   | ProvidersCharged charged -> 
                                        match this.InsuranceSavings with
                                        | InsuranceSavings savings -> charged - savings
\$\endgroup\$
3
\$\begingroup\$

Yes, there are, indeed, a couple of code smells.

The first one is that TotalSavings is a type member, and not a function ;)

The other code smell is that you have the beginning of arrow code in your implementation.

I'd write it like this instead:

let totalSavings summary =
    let (ProvidersCharged charged) = summary.ProvidersCharged
    let (InsuranceSavings savings) = summary.InsuranceSavings
    charged - savings

Notice that you don't have to use the match keyword when doing pattern match. With single-case discriminated unions, you can also pattern match directly into let-bound values.

Normally, the brackets aren't necessary, but in this case they are, because e.g. ProvidersCharged is both a case constructor and a record label. If the case constructor had been unambiguous, you could have omitted the brackets, like this:

let Foo foo = bar

You can also pattern match directly in function arguments, but in this case, it becomes too verbose:

let totalSavings' { ProvidersCharged = ProvidersCharged charged; InsuranceSavings = InsuranceSavings savings; Claims = _ } =
    charged - savings

Notice how the record labels are automatically destructured here, using nested pattern matching: matching is done first on the record labels, then on the case constructors inside of the labels.

You'll also notice that I used a wildcard match on Claims because I don't care about that value.

|improve this answer|||||
\$\endgroup\$
0
\$\begingroup\$

I took Mark's answer and refactored my original code to the following:

type ClaimsSummary = 
    { Member:IdCard
      Claims:Claim seq
      ProvidersCharged:ProvidersCharged
      InsuranceSavings:InsuranceSavings 
    } member this.TotalSavings() =
        let (ProvidersCharged charged) = this.ProvidersCharged
        let (InsuranceSavings savings) = this.InsuranceSavings
        charged - savings

I believe that TotalSavings is context specific to the record type. Thus, I made the decision to tightly couple the TotalSavings() "member" to the ClaimsSummary record type. Hence, I don't think this function would be used by any other clients based on the summary typed argument used.

|improve this answer|||||
\$\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.