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

Idiomatic and Conventional F#

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

Problem

As of late I have been learning F#. Today I accumulated what I have learned so far to develop my first program - a script that counts the number of lines of code in a given Visual Studio project.

The code for said program is as follows:

#r "System.Xml.Linq"

open System
open System.IO
open System.Xml.Linq
open System.Text.RegularExpressions

printf "Enter solution path: "
let solutionPath = Console.ReadLine()
let solutionDirPath = Path.GetDirectoryName(solutionPath)

let solutionText = File.ReadAllText(solutionPath)
let projectPaths  = 
    Regex.Matches(solutionText, "([^\"]*csproj)")
    |> Seq.cast
    |> Seq.map (fun m -> Path.Combine(solutionDirPath, m.Groups.[1].Value))

let codeFilePaths = 
    projectPaths
    |> Seq.map (fun projPath -> 
        let projectDirPath = Path.GetDirectoryName(projPath)
        let document = XDocument.Load(projPath)
        document.Descendants()
            |> Seq.where (fun desc -> desc.Name.LocalName = "Compile")
            |> Seq.map (fun desc -> Path.Combine(projectDirPath, desc.FirstAttribute.Value)))
        |> Seq.concat

let locIsSignificant (loc : string) =
    let sanitizedLoc = loc.Trim()
    match sanitizedLoc with
    | ""  -> false
    | "{" -> false
    | "}" -> false
    | _   -> true

let totalLoc = 
    codeFilePaths
    |> Seq.sumBy (fun path ->
        File.ReadLines(path)
        |> Seq.where locIsSignificant
        |> Seq.length)

printfn "Total Loc: %i" totalLoc
Console.ReadKey() |> ignore


I have tried write both clean and idiomatic code but I still have some concerns, namely:

  • In this script, I introduce sequential intermediate bindings (projectPaths->codeFilePaths->totalLoc) however, the binding locIsSignificant feels like an interruption to this sequence. Have I arranged my code in an conventional way?



  • Is my code as idomatic as can be? That is, could I make the code more expressive by using constructs or commands that I have not considered?



Of course, any additional feedb

Solution

Regex.Matches(solutionText, "([^\"]*csproj)")
|> Seq.cast
|> Seq.map (fun m -> Path.Combine(solutionDirPath, m.Groups.[1].Value))


Why are you using groups? Since the whole match is a single group, you can remove the parentheses from the expression and then directly use m.Value.

Also, you should probably check that the name ends in .csproj, not just csproj and that it's actually the end of the file name.

let codeFilePaths = 
    projectPaths
    |> Seq.map (fun projPath -> 
        let projectDirPath = Path.GetDirectoryName(projPath)
        let document = XDocument.Load(projPath)
        document.Descendants()
            |> Seq.where (fun desc -> desc.Name.LocalName = "Compile")
            |> Seq.map (fun desc -> Path.Combine(projectDirPath, desc.FirstAttribute.Value)))
        |> Seq.concat


-
I think the complicated map lambda makes this hard to read, I would instead write it as a local named function. If you don't want to do that, at least make sure to indent your code properly (the first |> Seq.map and the |> Seq.concat should be on the same level).

-
Instead of the where, you can use the overload of Descendants that accepts a node name (but this means you have to use XML namespaces).

-
Instead of relying on the first attribute being the right one, I would access the attribute by name.

With all these changes and John Palmer's suggestion about Seq.collect, the code becomes:

let codeFilePaths = 
    let codeFilesForProject projPath = 
        let projectDirPath = Path.GetDirectoryName(projPath)
        let document = XDocument.Load(projPath)
        let ns = XNamespace.Get "http://schemas.microsoft.com/developer/msbuild/2003"
        document.Descendants(ns + "Compile")
            |> Seq.map (fun desc -> Path.Combine(projectDirPath, desc.Attribute(XName.Get "Include").Value))
    projectPaths |> Seq.collect codeFilesForProject

Code Snippets

Regex.Matches(solutionText, "([^\"]*csproj)")
|> Seq.cast<Match>
|> Seq.map (fun m -> Path.Combine(solutionDirPath, m.Groups.[1].Value))
let codeFilePaths = 
    projectPaths
    |> Seq.map (fun projPath -> 
        let projectDirPath = Path.GetDirectoryName(projPath)
        let document = XDocument.Load(projPath)
        document.Descendants()
            |> Seq.where (fun desc -> desc.Name.LocalName = "Compile")
            |> Seq.map (fun desc -> Path.Combine(projectDirPath, desc.FirstAttribute.Value)))
        |> Seq.concat
let codeFilePaths = 
    let codeFilesForProject projPath = 
        let projectDirPath = Path.GetDirectoryName(projPath)
        let document = XDocument.Load(projPath)
        let ns = XNamespace.Get "http://schemas.microsoft.com/developer/msbuild/2003"
        document.Descendants(ns + "Compile")
            |> Seq.map (fun desc -> Path.Combine(projectDirPath, desc.Attribute(XName.Get "Include").Value))
    projectPaths |> Seq.collect codeFilesForProject

Context

StackExchange Code Review Q#63420, answer score: 3

Revisions (0)

No revisions yet.