Max
Max

Reputation: 869

Pass Function to reduce duplicate code

I'm trying to learn F# and I feel like I can write / rewrite this block of code to be more "idiomatic" F# but I just can't figure out how I can accomplish it.

My simple program will be loading values from 2 csv files: A list of Skyrim potion effects, and a list of Skyrim Ingredients. An ingredient has 4 Effects. Once I have the Ingredients, I can write something to process them - right now, I just want to write the CSV load in a way that makes sense.

Code

Here are my types:

type Effect(name:string, id, description, base_cost, base_mag, base_dur, gold_value) =
    member this.Name = name
    member this.Id = id
    member this.Description = description
    member this.Base_Cost = base_cost
    member this.Base_Mag = base_mag
    member this.Base_Dur = base_dur
    member this.GoldValue = gold_value

type Ingredient(name:string, id, primary, secondary, tertiary, quaternary, weight, value) =
    member this.Name = name
    member this.Id = id
    member this.Primary = primary
    member this.Secondary = secondary
    member this.Tertiary = tertiary
    member this.Quaternary = quaternary
    member this.Weight = weight
    member this.Value = value

Here is where I parse an individual comma-separated string, per type:

let convertEffectDataRow (csvLine:string) =
    let cells = List.ofSeq(csvLine.Split(','))
    match cells with
    | name::id::effect::cost::mag::dur::value::_ ->            
        let effect = new Effect(name, id, effect, Decimal.Parse(cost), Int32.Parse(mag), Int32.Parse(dur), Int32.Parse(value))
        Success effect
    | _ -> Failure "Incorrect data format!"


let convertIngredientDataRow (csvLine:string) =
    let cells = List.ofSeq(csvLine.Split(','))
    match cells with
        | name::id::primary::secondary::tertiary::quaternary::weight::value::_ ->
            Success (new Ingredient(name, id, primary, secondary, tertiary, quaternary, Decimal.Parse(weight), Int32.Parse(value)))
        | _ -> Failure "Incorrect data format!"

So I feel like I should be able to build a function that accepts one of these functions or chains them or something, so that I can recursively go through the lines in the CSV file and pass those lines to the correct function above. Here is what I've tried so far:

type csvTypeEnum = effect=1 | ingredient=2        

let rec ProcessStuff lines (csvType:csvTypeEnum) =
    match csvType, lines with
        | csvTypeEnum.effect, [] -> []
        | csvTypeEnum.effect, currentLine::remaining ->
            let parsedLine = convertEffectDataRow2 currentLine
            let parsedRest = ProcessStuff remaining csvType
            parsedLine :: parsedRest
        | csvTypeEnum.ingredient, [] -> []
        | csvTypeEnum.ingredient, currentLine::remaining ->
            let parsedLine = convertIngredientDataRow2 currentLine
            let parsedRest = ProcessStuff remaining csvType
            parsedLine :: parsedRest
        | _, _ -> Failure "Error in pattern matching"

But this (predictably) has a compile error on second instance of recursion and the last pattern. Specifically, the second time parsedLine :: parsedRest shows up does not compile. This is because the function is attempting to both return an Effect and an Ingredient, which obviously won't do.

Now, I could just write 2 entirely different functions to handle the different CSVs, but that feels like extra duplication. This might be a harder problem than I'm giving it credit for, but it feels like this should be rather straightforward.

Sources

The CSV parsing code I took from chapter 4 of this book: https://www.manning.com/books/real-world-functional-programming

Upvotes: 1

Views: 110

Answers (3)

TheInnerLight
TheInnerLight

Reputation: 12184

Since the line types aren't interleaved into the same file and they refer to different csv file formats, I would probably not go for a Discriminated Union and instead pass the processing function to the function that processes the file line by line.

In terms of doing things idiomatically, I would use a Record rather than a standard .NET class for this kind of simple data container. Records provide automatic equality and comparison implementations which are useful in F#.

You can define them like this:

type Effect = {
    Name : string; Id: string; Description : string; BaseCost : decimal; 
    BaseMag : int; BaseDuration : int; GoldValue : int
    }

type Ingredient= {
    Name : string; Id: string; Primary: string; Secondary : string; Tertiary : string; 
    Quaternary : string; Weight : decimal; GoldValue : int
    }

That requires a change to the conversion function, e.g.

let convertEffectDataRow (csvLine:string) =
    let cells = List.ofSeq(csvLine.Split(','))
    match cells with
    | name::id::effect::cost::mag::dur::value::_ ->            
        Success  {Name = name; Id = id; Description = effect;  BaseCost = Decimal.Parse(cost); 
                  BaseMag = Int32.Parse(mag); BaseDuration = Int32.Parse(dur); GoldValue = Int32.Parse(value)}
    | _ -> Failure "Incorrect data format!"

Hopefully it's obvious how to do the other one.

Finally, cast aside the enum and simply replace it with the appropriate line function (I've also swapped the order of the arguments).

let rec processStuff f lines  =
    match lines with
    |[] -> []
    |current::remaining -> f current :: processStuff f remaining

The argument f is just a function that is applied to each string line. Suitable f values are the functions we created above, e.g.convertEffectDataRow. So you can simply call processStuff convertEffectDataRow to process an effect file and processStuff convertIngredientDataRow to process and ingredients file.

However, now we've simplified the processStuff function, we can see it has type: f:('a -> 'b) -> lines:'a list -> 'b list. This is the same as the built-in List.map function so we can actually remove this custom function entirely and just use List.map.

let processEffectLines lines = List.map convertEffectDataRow lines

let processIngredientLines lines = List.map convertIngredientDataRow lines

Upvotes: 2

Mikhail Shilkov
Mikhail Shilkov

Reputation: 35144

  1. (optional) Convert Effect and Ingredient to records, as s952163 suggested.
  2. Think carefully about the return types of your functions. ProcessStuff returns a list from one case, but a single item (Failure) from the other case. Thus compilation error.
  3. You haven't shown what Success and Failure definitions are. Instead of generic success, you could define the result as

    type Result = 
      | Effect of Effect 
      | Ingredient of Ingredient 
      | Failure of string
    

And then the following code compiles correctly:

let convertEffectDataRow (csvLine:string) =
    let cells = List.ofSeq(csvLine.Split(','))
    match cells with
    | name::id::effect::cost::mag::dur::value::_ ->            
        let effect = new Effect(name, id, effect, Decimal.Parse(cost), Int32.Parse(mag), Int32.Parse(dur), Int32.Parse(value))
        Effect effect
    | _ -> Failure "Incorrect data format!"


let convertIngredientDataRow (csvLine:string) =
    let cells = List.ofSeq(csvLine.Split(','))
    match cells with
        | name::id::primary::secondary::tertiary::quaternary::weight::value::_ ->
            Ingredient (new Ingredient(name, id, primary, secondary, tertiary, quaternary, Decimal.Parse(weight), Int32.Parse(value)))
        | _ -> Failure "Incorrect data format!"

type csvTypeEnum = effect=1 | ingredient=2        

let rec ProcessStuff lines (csvType:csvTypeEnum) =
    match csvType, lines with
    | csvTypeEnum.effect, [] -> []
    | csvTypeEnum.effect, currentLine::remaining ->
        let parsedLine = convertEffectDataRow currentLine
        let parsedRest = ProcessStuff remaining csvType
        parsedLine :: parsedRest
    | csvTypeEnum.ingredient, [] -> []
    | csvTypeEnum.ingredient, currentLine::remaining ->
        let parsedLine = convertIngredientDataRow currentLine
        let parsedRest = ProcessStuff remaining csvType
        parsedLine :: parsedRest
    | _, _ -> [Failure "Error in pattern matching"]

csvTypeEnum type looks fishy, but I'm not sure what you were trying to achieve, so just fixed the compilation errors.

Now you can refactor your code to reduce duplication by passing functions as parameters when needed. But always start with types!

Upvotes: 2

s952163
s952163

Reputation: 6324

You can certainly pass a function to another function and use a DU as a return type, for example:

type CsvWrapper =
    | CsvA of string
    | CsvB of int

let csvAfunc x =
    CsvA x

let csvBfunc x =
    CsvB x

let csvTopFun x  =
    x 

csvTopFun csvBfunc 5
csvTopFun csvAfunc "x"

As for the type definitions, you can just use records, will save you some typing:

type Effect = { 
    name:string 
    id: int 
    description: string
}
let eff = {name="X";id=9;description="blah"}

Upvotes: 0

Related Questions