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

Parsing sections from a log file using F#

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

Problem

After reading a lot about F#, I am today cutting my very first real life F# code and I'd appreciate any feedback on my approach.

I'm writing a simple utility to analyse a log file. The code below represents my approach for identifying sections of the log and representing them in a structure. The "real life" situation is to find application sessions in the log file. The example below model this by parsing a list of numeric strings!

The main areas of relevance are the GetSection and GetSections methods below. My questions are:

  • Is my approach sensible?



  • Is this functional and idiomatic?



  • Is there an alternative approach that would be clearer or more elegant?



There is a LinqPad version of the code available here: http://share.linqpad.net/99khmx.linq

```
// Here are the "lines" from my "log file".
let lines = [1 .. 25] |> List.map (fun x -> x.ToString())

// For this exercise, section criteria is divisibility by 5.
// val DivisibleByFive : s:string -> bool
let DivisibleByFive (s:string) =
System.Int32.Parse(s) % 5 = 0

// I am going to use the following type to hold the information
// from each section of the log.
type Section (lines:string list) =
member x.Lines = lines

// Read a single section from the top of the log (based on: http://fssnip.net/iV).
// val GetSection : lines:string list -> Section * string list
let GetSection (lines:string list) =
let rec getSection (l:string list) result =
match l with
| h::t when DivisibleByFive h ->
let values = h::result |> List.rev
(new Section(values), t)
| h::t when not (DivisibleByFive h) ->
getSection t (h::result)
| _ ->
(new Section(result), [])
getSection lines []

// Get a list of all sections from the log.
// val GetSections : lines:string list -> Section list
let GetSections (lines:string list) =
let rec getSections (l:string list) result =
match GetSection l with
| (a,

Solution

Welcome to the world of F#! Your code is looking good, and thanks for posting the LinqPad file. Here are a couple of pointers:

  • sprintf ... |> System.Console.WriteLine can be replaced by a call to printfn.



  • The variable name i in the for loop is misleading, as it looks like an index. Calling it section would be better.



  • If you don't need the result of an expression, such as System.Console.ReadLine, it's common to name the variable _.



Then your main function would look like this:

let main argv = 
    let sections = GetSections lines
    printfn "Parsed %d section from the log:" sections.Length
    for section in sections do
        printfn "  Section starting with %s" section.Lines.Head
    let _ = System.Console.ReadLine()
    0


You could also write the for loop using List.iter, but it's a matter of personal preference:

sections |> List.iter (fun section -> printfn "  Section starting with %s" section.Lines.Head)


Now to the main part of your code. I would say that it's functional and idiomatic, but in my experience with F#, sometimes it's best to compromise on the functional approach.

For example, consider GetSections. The basic idea is to go through a sequence, building up lists until a certain predicate is met. I feel that the functional approach, with its list reversal, obscures the intention of the code. Here is what I would propose:

Edit Fixed a bug where last elements might not be returned.

/// 
/// Splits the given sequence into non-empty contiguous subsequences, such
/// that the last element of every subsequence (except possibly the last)
/// satisfies the given predicate. No other elements satisfy the predicate.
/// 
/// 
/// 
/// let even x = x % 2 = 0
/// printfn "%A" (splitAtEach even (seq { 1 .. 3 }))
/// printfn "%A" (splitAtEach even (seq { 1 .. 4 }))
/// 
/// The output is:
/// 
/// seq [[1; 2]; [3]]
/// seq [[1; 2]; [3; 4]]
/// 
/// 
let splitAtEach (predicate : 'T -> bool) (xs : seq) : seq =
    let section = new ResizeArray()
    seq {
        for x in xs do
            section.Add x
            if predicate x then
                yield Seq.toList section
                section.Clear()
        if section.Any() then
            yield Seq.toList section
    }


Then GetSections could be written

let GetSections lines =
    splitAtEach DivisibleByFive lines
    |> Seq.map (fun lines -> new Section(lines))
    |> Seq.toList


Edit Found a bug in your definition of GetSections, where GetSections ["1"; "2"; "3"] = [["3"; "2"; "1"]].

Code Snippets

let main argv = 
    let sections = GetSections lines
    printfn "Parsed %d section from the log:" sections.Length
    for section in sections do
        printfn "  Section starting with %s" section.Lines.Head
    let _ = System.Console.ReadLine()
    0
sections |> List.iter (fun section -> printfn "  Section starting with %s" section.Lines.Head)
/// <summary>
/// Splits the given sequence into non-empty contiguous subsequences, such
/// that the last element of every subsequence (except possibly the last)
/// satisfies the given predicate. No other elements satisfy the predicate.
/// </summary>
/// <example>
/// <c>
/// let even x = x % 2 = 0
/// printfn "%A" (splitAtEach even (seq { 1 .. 3 }))
/// printfn "%A" (splitAtEach even (seq { 1 .. 4 }))
/// </c>
/// The output is:
/// <c>
/// seq [[1; 2]; [3]]
/// seq [[1; 2]; [3; 4]]
/// </c>
/// </example>
let splitAtEach (predicate : 'T -> bool) (xs : seq<'T>) : seq<'T list> =
    let section = new ResizeArray<'T>()
    seq {
        for x in xs do
            section.Add x
            if predicate x then
                yield Seq.toList section
                section.Clear()
        if section.Any() then
            yield Seq.toList section
    }
let GetSections lines =
    splitAtEach DivisibleByFive lines
    |> Seq.map (fun lines -> new Section(lines))
    |> Seq.toList

Context

StackExchange Code Review Q#55554, answer score: 3

Revisions (0)

No revisions yet.