Move files only if they are image format

Question

i just would like to be sure whether i missed something in my code or not. I want to validate before moving my files from one folder to another that file is image. I prepared this function and usegae like below. Can you please tell me is it ok? Do i need some dispose or anything else or its just quit enought. many thanks, cheers.

Function IsValidImage(filename As String) As Boolean
    Try
        Dim img As System.Drawing.Image = System.Drawing.Image.FromFile(filename)
        img.Dispose()
    Catch generatedExceptionName As OutOfMemoryException
        ' Image.FromFile throws an OutOfMemoryException  
        ' if the file does not have a valid image format or 
        ' GDI+ does not support the pixel format of the file. 
        ' 
        Return False
    End Try
    Return True
End Function

If IsValidImage("c:\path\to\your\file.ext") Then
    'do something
    '
Else
    'do something else
    '
End If

Using exceptions to control flow is a bad practice. Better to examine the binary file and see if it matches known image formats. — Tim, Nov 04 '14 at 08:02
See this [answer](http://stackoverflow.com/a/7906071/745969) for some possible ways to do this. — Tim, Nov 04 '14 at 08:03
I would add the line `img.Dispose()` at the end of your `try` to ensure the resources are cleared when you no longer need them. — Nunners, Nov 04 '14 at 08:04
i've added image.Dispose() in the end. Should it be ok right now? — Bob, Nov 04 '14 at 08:10
@Bob - I would still say that this is not good code, because you're using exceptions to control it. Test the files to see if they're images, don't rely on (costly) exception throwing/raising. — Tim, Nov 04 '14 at 08:11
Using exceptions for control flow is not always a bad practice when you avoid writing hundreds of lines of code by that. Also I suspect the check function for a valid image would also take its time **and** must be developed first. So if you consider this a bad practice please show use at least one working alternative before yelling at somebody who got a working solution. — abto, Nov 04 '14 at 08:23
should i move Dim img as above Try clasue and add img.Dospise in catch additionaly or its not required? — Bob, Nov 04 '14 at 08:28
@abto - I disagree. Exceptions are just that - *exceptions*. They shouldn't be used in the place of code that can do the job - with that logic should I throw a file not found exception when I can check to see if the file exists?. The logic and code for checking for a valid image is well known and easily googled. I posted a link to an answer here on SO that addresses that in my 2nd comment. And I wasn't yelling at OP or anyone. OP asked if the function was ok - I gave my opinion. You're free to disagree. I gave my opinion and now I'm done. Have a nice day :) — Tim, Nov 04 '14 at 09:00

score 0 · Answer 1 · answered Nov 04 '14 at 09:03

0

Let the file path be "D:\web\sample\Image\my.Image.png" then you can check whether the file is an image file or not using the following code:

    Dim filepath As String = "D:\web\sample\Image\my.Image.png"
    Dim imageExtensions() As String = {"bmp", "gif", "jpg", "png", "psd", "psp", "thm", "tif", "yuv"}
    Dim pat As String = "\\(?:.+)\\(.+)\.(.+)"
    Dim r As Regex = New Regex(pat)
    Dim m As Match = r.Match(filepath)
    If imageExtensions.Contains(m.Groups(2).Captures(0).ToString()) Then
        MsgBox("valid Image")
    End If

answered Nov 04 '14 at 09:03

You can create a text file and save it as `pic.jpg`, but that doesn't make it an image file. – Tim Nov 04 '14 at 09:28
@tim : nice question, have to think more about it – Nov 04 '14 at 09:36

score 0 · Answer 2 · answered Nov 04 '14 at 10:43

You are on the right way, but you should make up your mind what you want to whant to gain:

If you want to develop a method telling you wheter the content of a file is considered a .Net recognized picture image, your approach is totally fine.

I start with your (slightly) overworked code:

Function IsValidImage(filename As String) As Boolean
    Try
        Using img As System.Drawing.Image = System.Drawing.Image.FromFile(filename)

            ' the file could be opened as image so it is valid
            Return True

        End Using
    Catch notValidException As OutOfMemoryException

        ' Image.FromFile throws an OutOfMemoryException  
        ' if the file does not have a valid image format or 
        ' GDI+ does not support the pixel format of the file. 
        ' 
        Return False

    End Try
    ' Every other Exception is considered not to be able too look whether the
    ' file is an Image or not, so it should be thrown to outside
End Function

Don't misjudge this approach as using Exception for control flow, you are only reacting to the one thrown if the content of the file is not usable for creating an Image. This let's the decision on the framework whether the file is valid for an Imageor not.

Let's refine your Method a little bit, so that you can determine wheter the image in the file does not exceed a given size:

Function IsValidImage(filename As String, maxSize As Size) As Boolean
    Try
        Using img As System.Drawing.Image = System.Drawing.Image.FromFile(filename)

            ' Returns True if the image is smaller or equal than maxSize
            Return img.Size.Width <= maxSize.Width AndAlso
                img.Size.Height <= maxSize.Height

        End Using
    Catch notValidException As OutOfMemoryException

        Return False

    End Try
End Function

Also this is not a misuse of Exceptions. It is a practice of fail fast: if I can't get an Image the file is not an Image. Otherwise I check the dimension on the opened Image, making my outcome dependent on operations done with it.

Misusing an Exception for control flow could, but must not be the following:

Sub CheckImage(filename As String)
    Try
        Using img As System.Drawing.Image = System.Drawing.Image.FromFile(filename)

        End Using
    Catch notValidException As OutOfMemoryException

        Throw New FileIsNoImageException()

    End Try
End Sub

Try
    CheckImage("c:\path\to\your\file.ext")
    'do something
    '
Catch invalid As FileIsNoImageException
    'do something else
    '
End Try

Even this approach has a valid usage if you consider a file not beeing an Image as an error. But normaly you would do this in functions which give you the Image as return value.

But this is a tottaly no go:

Sub CheckImage(filename As String)
    Try
        Using img As System.Drawing.Image = System.Drawing.Image.FromFile(filename)

        Throw New FileIsImageException() ' DON'T DO SUCH A CRAP

        End Using
    Catch notValidException As OutOfMemoryException

        Throw New FileIsNoImageException()

    End Try
End Sub

Try
    CheckImage("c:\path\to\your\file.ext")
Catch valid As FileIsImageException
    'do something
    '
Catch invalid As FileIsNoImageException
    'do something else
    '
End Try

@Tim, I beg your pardon, but I can't hear this Exception use is a bad practice tale any more.

Move files only if they are image format

2 Answers2