4
\$\begingroup\$

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<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 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 feedback is welcome/encouraged.

\$\endgroup\$
2
\$\begingroup\$
Regex.Matches(solutionText, "([^\"]*csproj)")
|> Seq.cast<Match>
|> 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
\$\endgroup\$
2
\$\begingroup\$

This started as a comment and grew a little longer

I would maybe put let locIsSignificant (loc : string) = at the top of the file and change it to

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

which is a little briefer and I think also shows the logic better. In particular, it removes the auxiliary function from the logic flow by moving it to the top.

In codepaths, there are a few small changes that I would make.

replace

Seq.map ...
|> Seq.concat

with

Seq.collect

and change

        |> Seq.where (fun desc -> desc.Name.LocalName = "Compile")
        |> Seq.map (fun desc -> Path.Combine(projectDirPath, desc.FirstAttribute.Value)))

to

|> Seq.choose (fun desc -> 
    match desc.Name.LocalName with - 
    |"Compile" -> Some (Path.Combine(projectDirPath, desc.FirstAttribute.Value)) 
    | _ -> None))
\$\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.