5
\$\begingroup\$

The F# code below embodies the two specific questions I have. It is part of a working transformation utility, the entirety of which, including sample data, is in this gist: http://bit.ly/2i3kUwF.

Generically, the task is: given a file of discrete records, compute a value for a subsequent record that depends on a value of the previous record. Since I want a transformed stream and not just a computed summary value, this seems a perfect use of Seq.scan. Here is the relevant portion of main:

File.ReadLines(inFile)                  
    |> Seq.map appendEmptyColumnForBarValue  // add the col to hold the new barValue
    |> Seq.scan updateBarValue ("", target)  // compute the barValue, based on prev state and target
    |> Seq.skip 1                            // exclude initial blank state above from output 
    |> Seq.iter outFile.WriteLine

But I have some questions concerning proper usage of Seq.scan.

Since, by definition, there is no initial state prior to reading the first record, and because I want to return an entire record to the stream, I initialize state to an empty string (ignore the "target" for a bit - that is question 2). This creates the need to distinguish between this special case of the of the state being the empty string that first time through, vs all other invocations:

let updateBarValue (state:string, target:int32) (line:string) = 
    // This is the "folder" function to maintain state for the scan function in main.
    // Since the first time through there will be no accumulated state, the inital state is set to "".
    // The match below handles the decision for first time through, or all other cases.
    //
    // From doc, usage for Seq.scan:
    // Seq.scan folder state source
    //
    // Because scan accepts a single parameter for state AND we need the target passed from the command line
    // invocation, we bundle state and target into a tuple. 
    // However, target remains unchanged, while the true state is continually updated
    match state with
    | "" -> buildLineWithBarValue  (line, target) line  // first time through we pass line instead of accumulated state
    | _  -> buildLineWithBarValue (state, target) line    

And this use of the empty string, forces the

    |> Seq.skip 1                            // exclude initial blank state above from output 

in the main routine above to prevent a blank line from being output.

So, question #1: Am I using Seq.scan correctly? Is there a different/better way to accomplish the goal, that would not force the inclusion of the "Seq.skip 1", which seems less-than-tidy? This would be an easy-to-forget item in this pattern.

Secondly, let's deal with that "target" value. As you can see from the comment above, I'm cheating a bit by adding it to the state for Seq.scan because it is necessary for the computation and I saw no other way to pass it to my Seq.scan's folder function, updateBarValue.

So, question #2: it seems to me that passing target in as part of the state, when it is clearly not, is a code smell of some sort. How might this be achieved without my cheat?

The code works, but I'm not happy with it as is; looking for ideas for improvements (in the gist there are a couple of clear TODOs- not talking about those). And, of course, if there is something stupid/crazy that I have not asked about or flagged with a TODO, don't be shy.

\$\endgroup\$
  • \$\begingroup\$ This belongs on StackOverflow, not Code Review. \$\endgroup\$ – Fyodor Soikin Dec 24 '16 at 22:23
  • 3
    \$\begingroup\$ @FyodorSoikin Happy to move it, but not clear why it belongs there, rather than here. Given what I read about missions - (Stack Overflow helps you fix non-working code; Code Review helps you improve already working code) - it seemed more appropriate here... \$\endgroup\$ – akucheck Dec 24 '16 at 22:40
6
\$\begingroup\$

First question - no, there is no way to get rid of this "initial" value completely. There is, however, a way to make it more intuitive that the values is "missing" - the Option type. Pass None as initial state, and return Some from then on. This will also allow you to elegantly get rid of that initial value by using Seq.choose.

Second question - easy: just make target first parameter of the folder function and pass it in via partial application:

let updateBarValue target state (line:string) = 
   let effectiveState =
      match state with 
      | None -> line
      | Some s -> s
   Some (buildLineWithBarValue (effectiveState, target) line)

File.ReadLines(inFile) 
   |> Seq.map appendEmptyColumnForBarValue // add the col to hold the new barValue
   |> Seq.scan (updateBarValue target) None // compute the barValue, based on prev state and target
   |> Seq.choose id // get rid of the initial input
   |> Seq.iter outFile.WriteLine

As an aside, I noticed your tendency to tuple function arguments. That is generally considered bad form, unless arguments inherently "belong together" - e.g. point coordinates, - because curried arguments give more freedom. To this end, I would make buildLineWithBarValue's arguments curried as well.

\$\endgroup\$
  • \$\begingroup\$ This works very nicely now that I included the Seq.choose from your code above. Cleaner solution from every angle: intent clearer by using Option type; and I agree Seq.choose much better than my Seq.skip 1. In addition, I have removed my "over-tupling". Excellent suggestion. \$\endgroup\$ – akucheck Dec 25 '16 at 20:08
  • \$\begingroup\$ Glad I could help. \$\endgroup\$ – Fyodor Soikin Dec 25 '16 at 20:13

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.