current community

your communities

more stack exchange communities

Stack Exchange
tour help
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.
up vote 4 down vote favorite

I've been working on this module with the assistance of codereview, and based on the responses and my own research, this is my project so far. The goal behind the code is that it starts with a .NET bitmap object, extracts the byte array of image data from the bitmap, then transforms it into an array of arrays or matrix.

The idea behind putting the bitmap data into the matrix is that I want to be able to use a simple representation of a coordinate, like Array[10][20], to get the RGB value for the pixel located at the (10, 20) coordinate. Rather than using a proper 2D array, I'm choosing to use an array of arrays, or "jagged" array, for speed concerns.

Your assistance with sanity, style, F# conventions, and any other problems you spot is appreciated in helping further refine this algorithm. 

open System
open System.Drawing
open System.Drawing.Imaging
open System.Runtime.InteropServices


module ImageSearch = 
    let LoadBitmapIntoArray (pBitmap:Bitmap) =
        let tBitmapData = pBitmap.LockBits( Rectangle(Point.Empty, pBitmap.Size), 
                                            ImageLockMode.ReadOnly, 
                                            PixelFormat.Format24bppRgb )  

        let tImageArrayLength = Math.Abs(tBitmapData.Stride) * pBitmap.Height
        let tImageDataArray = Array.zeroCreate<byte> tImageArrayLength

        Marshal.Copy(tImageDataArray, 0, tBitmapData.Scan0, tImageArrayLength)
        pBitmap.UnlockBits(tBitmapData)

        pBitmap.Width, tImageDataArray

    let Transform2D ( (pArrayWidth:int), (pArray:byte[]) ) = 
        let tHeight = pArray.Length / ( pArrayWidth * 3 ) // 3 bytes per RGB

        let tImageArray = [|
            for tHeightIndex in [ 0 .. tHeight - 1] do
                let tStart  = tHeightIndex * pArrayWidth
                let tFinish = tStart + pArrayWidth - 1 
                yield [|    
                    for tWidthIndex in [tStart .. 3 .. tFinish] do
                        yield ( pArray.[tWidthIndex]
                              , pArray.[tWidthIndex + 1] 
                              , pArray.[tWidthIndex + 2] )
                |]
        |]

        tImageArray

    let SearchBitmap (pSmallBitmap:Bitmap) (pLargeBitmap:Bitmap) = 

        let tSmallArray = Transform2D <| LoadBitmapIntoArray pSmallBitmap 
        let tLargeArray = Transform2D <| LoadBitmapIntoArray pLargeBitmap

        let mutable tMatchFound = false
share|improve this question
    
a square "jagged" array <-- does not make much sense to me.... just saying. –  rolfl♦ Jul 21 at 22:53
    
Yeah I was just thinking about how to rephrase that. "An array of arrays where each array is the same length" maybe would work better? –  Kenneth Posey Jul 21 at 22:54
    
'matrix' may be a word that makes sense here. –  rolfl♦ Jul 21 at 22:55
    
I like that, thanks. –  Kenneth Posey Jul 21 at 22:55
    
It looks like your code has been cut off. let mutable tMatchFound = false, and then? –  mjolka Jul 22 at 0:27

1 Answer 1

up vote 1 down vote accepted

By convention, F# function names are camelCase, for example transform2D instead of Transform2D.

Hungarian notation is dead and buried. For example, use bitmap instead of pBitmap.

If ImageSearch is a top-level module, declarations do not have to be indented.

Your for...in expressions can be written without brackets. For example, instead of

for tHeightIndex in [ 0 .. tHeight - 1] do

write

for heightIndex in 0 .. height - 1 do

Similarly, instead of

for tWidthIndex in [tStart .. 3 .. tFinish] do

write

for widthIndex in start .. 3 .. finish do

This is not just a style issue, the different versions actually compile to different IL.

The final expression of a function is its return value, so instead of

let tHeight = ...
let tImageArray = [| ... |]
tImageArray

you can write

let height = ...
[| ... |]

In type annotations, it's conventional to have a space on either side of the :, for example instead of

let Transform2D ( (pArrayWidth:int), (pArray:byte[]) ) =

write

let transform2D (width : int, array : byte[]) =

I can't actually back up this last point. MSDN and FSharp.Core don't seem to stick to one style, even in the same function

let isProperSuperset (x:Set<'T>) (y: Set<'T>) = SetTree.psubset x.Comparer y.Tree x.Tree
share|improve this answer
    
"This is not just a style issue, the different versions actually compile to different IL." What is the significance of the different way it compiles? –  Kenneth Posey Jul 22 at 16:19
1  
@KennethPosey it's the difference between a sequence and a list. One major difference is that a list must have all its elements in memory at once. As a silly example, try timing printfn "%A" (Seq.length {0L .. 10000000L} compared to printfn "%A" (Seq.length [0L .. 10000000L]). –  mjolka Jul 23 at 2:34
    
so to make sure I'm understanding this correctly, the sequence option (using for-in-do without square brackets) is likely going to perform better than the list option when iterating? –  Kenneth Posey Jul 23 at 16:15
    
I just ran it through 100000000L for each of those two, and the sequence completed nearly instantly while the list hung for several seconds then overflowed. That answers my question then. Thanks a ton for the clarification. –  Kenneth Posey Jul 23 at 16:21

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.