Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

In my code I declared a dictionary to store various counters

let counters = Dictionary<Type, int>()

Later, I need to generate several labels using these counters, like this:

  • First type a label is generated for a given type, it should consist of the type name itself:
    "MyType"
  • Subsequent label should be the type name, appended with the number of times a label was previously generated for that type, in parentheses:
    "MyType (2)"

Here's how I'm accomplishing this now:

let t : Type = ...

let label = 
    if (counters.ContainsKey(t)) then
        counters.[t] <- counter.[t] + 1
        sprintf "%s (%i)" t.Name counters.[t]
    else
        counters.Add(t, 1)
        t.Name

This is exactly how I would write it if I had to use C# (with the exception of the in-line if / else), but after I wrote it F#, it seems clumsy and a little messy. I suspect there is a much more elegant, idiomatic way of writing this in F#. Perhaps using a match expression?

share|improve this question
add comment

3 Answers

up vote 2 down vote accepted

Another improvement would be to close over counters, assuming it's only used by this function.

let mkLabel =
  let counters = Dictionary()
  fun (ty: Type) ->
    match counters.TryGetValue(ty) with
    | true, curCount ->
      let newCount = curCount + 1
      counters.[ty] <- newCount
      sprintf "%s (%i)" ty.Name newCount
    | _ ->
      counters.Add(ty, 1)
      ty.Name

//Usage
mkLabel typeof<int>
mkLabel typeof<string>
share|improve this answer
    
Good suggestion. Yes, counters is only used for the purpose of generating labels in this function. –  p.s.w.g Dec 13 '13 at 22:40
add comment

Thanks to the input provided in the other answers, here's what I've come up with:

let label = 
    let (found, value) = this.counters.TryGetValue(t)
    this.counters.[t] <- value + 1
    if found 
    then (sprintf "%s (%i)" t.Name (value + 1)) 
    else t.Name

Or using a match expression

let label = 
    let (found, value) = this.counters.TryGetValue(t)
    this.counters.[t] <- value + 1
    match found with 
    | true -> (sprintf "%s (%i)" t.Name (value + 1)) 
    | false -> t.Name

I've also refactored this into a separate function, so the actual assignment is just

let label = getNextLabel(t)

Both of these look better than my original code, though I'm not sure that the match has added any expressiveness to the code, so for now I'm keeping the plain old if / else statement. This is mostly just taking advantage of the built-in behavior of dictionaries in .NET (TryGetValue returns the default value if the key is not found, and the Item property's setter will perform an insert if necessary), but I'm still open to any suggestions on improving this more.

share|improve this answer
add comment

You can indeed use match, together with the fact that F# converts methods with out parameters (like Dictionary.TryGetValue()) to methods returning tuples:

let label =
    match counters.TryGetValue(t) with
    | (true, counter) ->
        counters.[t] <- counter + 1
        sprintf "%s (%i)" t.Name counter
    | (false, _) ->
        counters.Add(t, 2)
        t.Name

Though I'm not sure whether this is actually any better.

Few more notes:

  • I think that t should be a parameter of label.
  • Using a mutable collection like Dictionary in F# doesn't feel right. I guess the “right” way to do it in functional style would be something like:

    label (map : Map<Type, int>) (type : Type) : (Map<Type, int>, string)
    

    But this would lead to code that's even less clear.

share|improve this answer
    
Thanks for your input. Please see my answer below. Hopefully the rewrite is clear an idiomatic. –  p.s.w.g Dec 13 '13 at 22:22
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.