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

Generating the pretty bit at the end of a URL

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

Problem

I'm having a dip into F# and am attempting not to write it like it's C#.

One area that bothers me is using members of System.String. These often need to be chained together, but the only way to do so that I've seen so far is pretty much the same as C#.

For example, I've written a function to generate the pretty bit at the end of a URL (similar to SE):

module String =

    open System.Text.RegularExpressions

    ///Makes a string suitable for use as part of a URL: 
    ///Removes syntax, replaces spaces with dashes and lowers case, 
    ///then trims first word after 30 chars
    let prettify (x:string) =  

        let parsed = Regex.Replace(x.Trim(),"[^0-9a-zA-Z ]","").Replace(" ","-").ToLower()

        match String.length parsed with
            | m when m > 30 -> 
                match parsed.IndexOf("-",30-1) with
                    | p when p  parsed
                    | p -> parsed.[..p]
            | _ -> parsed


Is there a way to refactor this to be "better F#"? I'm particularly concerned about how to better handle the methods on System.String and Regex.Replace. (The match part is a little convoluted to handle the edge case of the last word being the one that takes it over 30 chars.)

Solution

F# does have a String module, but it's almost empty. If you want to make string manipulations look more idiomatic, you'll have write functions that wrap System.String methods yourself (quick search didn't find anything existing):

let trim (input:string) =
    input.Trim()

let regexReplace (pattern:string) (replacement:string) (input:string) =
    Regex.Replace(input, pattern, replacement)

let replace (pattern:string) (replacement:string) (input:string) =
    input.Replace(pattern, replacement)

let toLower (input:string) =
    input.ToLower()

…

let parsed =
    x
    |> trim
    |> regexReplace "[^0-9a-zA-Z ]" ""
    |> replace " " "-"
    |> toLower


match String.length parsed with
    | m when m > 30 -> …
    | _ -> parsed


You don't need to try to differentiate from C# so hard. if would be more appropriate here:

if String.length parsed > 30 then …
else parsed


match parsed.IndexOf("-",30-1) with
    | p when p  parsed
    | p -> parsed.[..p]


You can rewrite this to avoid having to reference p in both branches:

match parsed.IndexOf("-",30-1) with
    | p when p >= 0 -> parsed.[..p]
    | _ -> parsed


Also, use more descriptive names than p, like index or maybe pos.

Also, this code usually returns a string that's over 30 characters and ends with -. Is that what you intended?

Code Snippets

let trim (input:string) =
    input.Trim()

let regexReplace (pattern:string) (replacement:string) (input:string) =
    Regex.Replace(input, pattern, replacement)

let replace (pattern:string) (replacement:string) (input:string) =
    input.Replace(pattern, replacement)

let toLower (input:string) =
    input.ToLower()

…

let parsed =
    x
    |> trim
    |> regexReplace "[^0-9a-zA-Z ]" ""
    |> replace " " "-"
    |> toLower
match String.length parsed with
    | m when m > 30 -> …
    | _ -> parsed
if String.length parsed > 30 then …
else parsed
match parsed.IndexOf("-",30-1) with
    | p when p < 0 -> parsed
    | p -> parsed.[..p]
match parsed.IndexOf("-",30-1) with
    | p when p >= 0 -> parsed.[..p]
    | _ -> parsed

Context

StackExchange Code Review Q#57397, answer score: 5

Revisions (0)

No revisions yet.