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.

I testing F# code which calculates "nearness" of two N-dimensional points using a least square euclidean distance algorithm. The class library is written in F# and the calling will be from VB.NET. The code will eventually be used for scientific binary data, however I wanted a simple real world test environment.

I choose to create an rgb color table class based upon a list of color names along with their 3 digit rgb value. The rgb values will be used for the distance algorithm. The code works, however I'd like to make sure that the distance algorithm in written in an efficient manner. The code will be called heavily in the final package.

My VB code will create the class instance, adds colors to the color table and finally searches for the nearest match of an unnamed color. The results is DarkGoldenRod.

Dim GreenYellow() As Double = {173, 255, 47}
Dim Orange() As Double = {255, 127, 0}
Dim Lavender() As Double = {230, 230, 250}
Dim Magenta() As Double = {255, 0, 255}
Dim DarkGoldenRod() As Double = {184, 134, 11}

Dim test() As Double = {122, 122, 122}

Dim objFSColorTable As New MyFSColorTable("GreenYellow", GreenYellow)
objFSColorTable.AddEntry("Lavender", Lavender)
objFSColorTable.AddEntry("Orange", Orange)
objFSColorTable.AddEntry("Magenta", {255, 0, 255})
objFSColorTable.AddEntry("DarkGoldenRod", {184, 134, 11})

Dim name = objFSColorTable.ColorSearch(test)

The F# code contains the color table library which should be self explanatory

type MyColor = 
  { Name: string;
    Values: float[];
    }

type MyFSColorTable(name: string, rgb:float[]) = class

  let mutable myHash = Hashtable()
  let mutable CurName = name
  let mutable CurDist = 0.0

  let mutable CurColor = 
    { Name = name;
      Values = rgb;
      }

  let mutable CurList = [CurColor]

  member x.AddColor name rgb = CurList <- List.append CurList [{Name = name; Values = rgb}]
  member x.Name   = name

  // Euclidean distance between 2 vectors
  member x.Dist (V1: float[]) V2 =
    Array.zip V1 V2
      |> Array.map(fun (v1, v2) -> (v1 - v2) * (v1 - v2))
      |> Array.sum

  // Loop thru color hash and find the nearest match
  member x.ColorSearch (R1: float[]) = 
    let mutable mindist = x.Dist R1 CurList.Head.Values
    let mutable name =  CurList.Head.Name
    for i in (CurList)  do 
      let mutable s = 0.0 
      s <- x.Dist R1 i.Values
      if s < mindist then 
        mindist <- s
        name <- i.Name
    name

  // RGB Hash Table
  member x.RGBHash = myHash
  member x.AddEntry (name:string) (rgb: float[]) = x.AddColor name rgb

end
share|improve this question
add comment

1 Answer

type MyColor = 
  { Name: string;
    Values: float[];
    }

The usual syntax is to put the opening { right after =.

Also, why are you using floats (or Doubles in VB) for the values? Integers should be enough. And it might be easier to use existing Color type, either from System.Windows.Media or from System.Drawing. As an added advantage, you could reuse the existing definitions for various colors.

type MyFSColorTable(name: string, rgb:float[]) = class

Collection really shouldn't take its first member in constructor. Either take all colors in the constructor (which means you wouldn't need mutable collection inside), or have a method for adding new colors.

Also, using class here is not necessary. If you remove that, don't forget to also remove the end.

let mutable myHash = Hashtable()
let mutable CurName = name
let mutable CurDist = 0.0

These members don't seem to be used in the program, so you should remove them. Also, try to avoid using mutable.

let mutable CurColor = 
  { Name = name;
    Values = rgb;
    }

This is only used to support the first item from constructor, remove it.

let mutable CurList = [CurColor]

CurList is not a very good name. Don't use abbreviations like Cur, they are unclear. And in this case, it doesn't add anything. A better name would be Colors.

member x.AddColor name rgb = CurList <- List.append CurList [{Name = name; Values = rgb}]

Adding to the end of list is inefficient. You can add a value to the front of the list by using ::: {Name = name; Values = rgb}::Colors.

member x.Name   = name

This member is not used. And it's very confusing, because it returns the name of the first color that was added to the collection. Remove it.

member x.Dist (V1: float[]) V2 =

This function doesn't use any instance fields, so it should be static. And it should be probably also private (i.e. static let dist V1 V2 =)

Array.map(fun (v1, v2) -> (v1 - v2) * (v1 - v2))

You could avoid the repetition by using pown: Array.map (fun (v1, v2) -> pown (v1 - v2) 2).

// Loop thru color hash and find the nearest match
member x.ColorSearch (R1: float[]) = 
  1. The comment is inaccurate, the function doesn't use any hash.
  2. R1 is a really bad name. And ColorSearch is not a great name either, something like FindNearestColor would be much better.
  3. The whole method is written in a purely imperative style. If you're going to write this way, there pretty much isn't a reason why you should use F#, F# is a poor imperative language.

    A better way to write this method would be to use List.minBy:

member x.FindNearestColor (rgb : float[]) =
  let nearestColor =
    Colors |> List.minBy (fun color -> dist rgb color.Values)
  nearestColor.Name

Also, it seems you think searching through the list could be inefficient (because it seems you attempted to use a hash table). But you can't use hash table for something like this. You could use something like an R-tree, but implementing that would be much more complicated. So I would do that only if performance of this method was really bad.

Dim objFSColorTable As New MyFSColorTable("GreenYellow", GreenYellow)
objFSColorTable.AddEntry("Lavender", Lavender)
objFSColorTable.AddEntry("Orange", Orange)
objFSColorTable.AddEntry("Magenta", {255, 0, 255})
objFSColorTable.AddEntry("DarkGoldenRod", {184, 134, 11})

This is quite verbose. A better way would be to use collection initializer. How exactly would that look depends on how do you decide to fix initialization of MyFSColorTable.

share|improve this answer
    
svick, I appreciate the comments. I'm not sure if you wanted to say "Adding to the end of list is ineffective" or perhaps "inefficient", as you can add to the end of the list. And yes, I should have definitely cleaned up my work in progress a little more before posting. The "pown" and "List.MinBy" comments were exactly the type of insights I was looking for in working with a new language. I used the floating point because the rgb color was only a test for my bigger picture numerics. In fact, that is why I didn't use a built-in color. –  dgp Feb 4 '13 at 14:53
    
@dgp Yeah, I did mean inefficient (adding to the end of list is O(n), adding to the front is O(1)). –  svick Feb 4 '13 at 15:02
    
I revised the code, however I didn't see where to post revised code in the faq. –  dgp Feb 4 '13 at 16:12
    
@dgp You might want to have a look at the meta question Is it valid to reask (many) question on same script? –  svick Feb 4 '13 at 16:18
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.