current community

your communities

more stack exchange communities

Stack Exchange Inbox Reputation and Badges
tour help
Sign up ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 1 down vote favorite

I'm building this as part of a larger class to rapidly search for smaller images inside larger images. This is my first serious F# project so comments especially concerning style and organization are appreciated.

module imagesearch = 
    type BitmapSearchClass() = 
        // Largely inspired by this article
        // http://msdn.microsoft.com/en-us/library/system.drawing.imaging.bitmapdata.aspx
        static member loadBitmapIntoArray (pBitmap:Bitmap) =
            let tBitmapRectangle = Rectangle(0, 0, pBitmap.Width, pBitmap.Height)

            let tLockMode = ImageLockMode.ReadOnly
            let tImageFormat = PixelFormat.Format24bppRgb

            let tBitmapData = pBitmap.LockBits(tBitmapRectangle, tLockMode, tImageFormat) 
            let tIntPtr = tBitmapData.Scan0

            let tImageByteLength = Math.Abs(tBitmapData.Stride) * pBitmap.Height

            // Zerocreate creates the "empty" array
            let tImageRGBValues : byte array = Array.zeroCreate tImageByteLength

            do Marshal.Copy(tImageRGBValues, 0, tIntPtr, tImageByteLength)

            do pBitmap.UnlockBits(tBitmapData)

            tImageRGBValues, pBitmap.Width, pBitmap.Height

Current state of the module is below

module ImageSearch = 
    type BitmapSearch() = 
        // Largely inspired by this article
        // http://msdn.microsoft.com/en-us/library/system.drawing.imaging.bitmapdata.aspx
        static member 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)

            tImageDataArray, pBitmap.Width, pBitmap.Height
share|improve this question

1 Answer 1

up vote 1 down vote accepted 
module imagesearch =

All types should use PascalCase, this includes modules. So the correct name would be ImageSearch.

type BitmapSearchClass() =

That's a weird name, names of classes shouldn't end with Class. And why is it a class in the first place? If it contains only static members, it should probably be a module.

static member loadBitmapIntoArray (pBitmap:Bitmap) =

Public methods should also use PascalCase (LoadBitmapIntoArray).

pBitmap:Bitmap
let tBitmapRectangle = …

Prefixing each parameter with p and each local variable with t seems like a weird kind of Hungarian notation. Please don't do that, it doesn't serve any purpose.

Rectangle(0, 0, pBitmap.Width, pBitmap.Height)

A shorter way to write this would be:

Rectangle(Point.Empty, pBitmap.Size)

let tLockMode = ImageLockMode.ReadOnly
let tImageFormat = PixelFormat.Format24bppRgb

let tBitmapData = pBitmap.LockBits(tBitmapRectangle, tLockMode, tImageFormat)

Why are these variables? Since you never use them again, and since naming them doesn't add any useful information, you can just write them directly in the function call:

let tBitmapData = pBitmap.LockBits(tBitmapRectangle, ImageLockMode.ReadOnly, PixelFormat.Format24bppRgb)

let tImageRGBValues : byte array = Array.zeroCreate tImageByteLength

Another way to specify the type is to add type parameter to zeroCreate:

let tImageRGBValues = Array.zeroCreate<byte> tImageByteLength

do Marshal.Copy(tImageRGBValues, 0, tIntPtr, tImageByteLength)

do pBitmap.UnlockBits(tBitmapData)

You don't need to write do here. (Actually, I was surprised you can write do here.)

share|improve this answer
    
Prefixing each parameter with p and each local variable with t seems like a weird kind of Hungarian notation. Please don't do that, it doesn't serve any purpose. <- Just to address this, I'm a huge fan of using intellisense and autocomplete to speed up my development, and I find prefixing some values (p for parameters, t for local scoped, and a few others) with a single letter tends to make using the intellisense easier. It's a habit from C#. All the rest of what you said looks great though, thanks for the tips. –  Kenneth Posey Jul 16 '14 at 16:16
    
I updated my original post to conform to several of your suggestions and add some more improvements of my own. =) –  Kenneth Posey Jul 16 '14 at 16:31

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.