_IsValidFileType()

[Shaggi]

What? Calling SRE a lot slower is fucking crazy. Theres a good reason pcre is included in a ton of languages out there. Its powerful and fast.

A lot slower in the context, dude. Chill off. No matter how you twist and turn it, interpreting a small language like regexp is always going to be slower than a linear search for a dot. Which is why i mean that stringinstr is the right tool for the job. regexp has its use, ofcourse.

Don't use the tools built into autoit because some people might not understand how they work? Thats weak. Especially coming from someone whos using hex values in there For loops.

If you consider the launguage were using, and the job this function have to do, im sure you'll agree that it is overkill. 0xFFFF is (for me) a convenient way of writing 65535. I dont know what you find more "readable".

I also want to point out in that perfomance test, the main slowdown is coming from StringReplace(). Not SRE. If you change the code to

Func _IsValidType(ByRef $sFilePath, $sList = "bat;cmd;exe")
    If StringRegExp($sList, "[/:<>|]") Then Return SetError(1, 0, -1)
    Return StringRegExp($sFilePath, "(?i)^(" & ".*?." & StringReplace($sList, ";", "|.*?.", 2) & ")z")
EndFunc   ;==>_IsValidType
my results then become:

(scaled) Test data:
_isvalidtype: 3263.63947284885
isvalidtype: 1521.45284004219[/quote]
Good find 
[quote]Which is only 2x slower. And more so, in reality your function only gains micro seconds. In my opinion its not worth turning a 2 line function into 10 lines just to gain microseconds. [code='Single times']@@ Debug(62) : $test1 = 0.128203815394698
@@ Debug(70) : $test2 = 0.0692218683760509

You can optimize my function a lot more if you want, like you did with guiness's. if you use 69255's function, numbers turn out like this:

Assumed overhead: 255.780810900625

(small) Test data:

_isvalidtype: 1324.87687182289

isvalidtype: 564.974722769614

Assumed overhead: 234.744067460558

(scaled) Test data:

_isvalidtype: 4596.83166602593

isvalidtype: 509.395533968111

Which is still around 10x faster. And if you worry about my functions length, you can shrink it to one line, as you saw. - but in both cases at the cost of readability and as of such more "bad style".

Using the right algorithm is for me programming 101. Getting the right understand of algorithm complexity can help you program much better code, and imo it would be ignorant not too. And still, if people did not do this, you're welcome to have your computer work at half the speed (or ten times slower).

Edited February 1, 2012 by Shaggi

[guinness]

I also want to point out in that performance test, the main slowdown is coming from StringReplace(). Not SRE. If you change the code to

Nice idea, I suppose a third parameter could be added for the number of extensions (minus 1) that are being checked.

[Robjong]

Hi,

I came across this topic and I think the function can be optimized quite a bit.

The caret (^) has no real function, there is no need to return if any regex meta characters are present in the 'list' var and the way extensions are prepended with '|.*?.' makes for a very exhaustive pattern, because for every entry in the group costly matching needs to be done.

Here is my take on it, we need to match the last dot followed by the extension, which will be in the 'list' matching group, and then the string end.

An extension in the 'list' var might contain regex meta characters and therefor needs to be escaped, instead of calling StringRegExpReplace to prepend meta characters with a backslash we will turn the regex engine of for each extension in the group with the E and Q modifiers.

Func _IsValidType($sFilePath, $sList = "bat;cmd;exe")
    Return StringRegExp($sFilePath, ".(?i:Q" & StringReplace($sList, ";", "E|Q") & "E)z")
EndFunc   ;==>_IsValidType

It is shorter then the original, it's also faster and it will match any characters (except for Q).

@Shaggi

The timing test you provided has one major flaw, and the results should be disregarded, here is why.

The function _IsValidType handles the 'list' var ($sList) in the function every time it is called, the isvalidtype function uses the array created by StringSplit which is done once before the loop. If you modify the isvalidtype function to accept the same string as the second param and call StringSplit inside the function at every call the SRE method is faster.

The test I used and the results:

#_isValidType - The new StringRegExp method
#old_isValidType - The old StringRegExp method
#old_isValidType - The old StringInStr method

(small) Test data:
Assumed overhead: 360.180545972701
_isValidType: 946.429958502075
old_IsValidType: 1588.35091745413
older_IsValidType: 2860.05594720264

(scaled) Test data:
Assumed overhead: 361.250002499875
_isValidType: 3605.51788910554
old_IsValidType: 22781.9302184891
older_IsValidType: 6104.03551822409

expandcollapse popup

SmallTest()
ScaledTest()


Func SmallTest()
    $types = "cmd;"
    $path = "C:"
    $ext = "wanted_file.err" ;extension is not in the list, so both algorithms have to search everything through
    $path &= $ext

    ;Calculate the overhead for the loop and a lot of func calls
    $testtime = TimerInit()
    For $i = 0 To 0xFFFF
        testfunc($path, $types)
    Next
    $overhead = TimerDiff($testtime)

    ;test of older_IsValidType
    $starttime = TimerInit()
    For $i = 0 To 0xFFFF
        older_IsValidType($path, $types)
    Next
    $test1 = TimerDiff($starttime)

    ;test of old_IsValidType
    $starttime = TimerInit()
    For $i = 0 To 0xFFFF
        old_IsValidType($path, $types)
    Next
    $test2 = TimerDiff($starttime)

    ;test of _IsValidType
    $starttime = TimerInit()
    For $i = 0 To 0xFFFF
        _IsValidType($path, $types)
    Next
    $test3 = TimerDiff($starttime)

    ConsoleWrite("(small) Test data: " & @CRLF & _
            "Assumed overhead: " & $overhead & @CRLF & _
            "_isValidType: " & $test3 - $overhead & @CRLF & _
            "old_IsValidType: " & $test2 - $overhead & @CRLF & _
            "older_IsValidType: " & $test1 - $overhead & @CRLF)

EndFunc   ;==>SmallTest

Func ScaledTest()
    $types = "cmd;bat;tiff;jpg;con;dll;exe;bat;pif;txt;xml;doc;bmp;mp3;mov;png;flp;cmd;bat;tiff;jpg;con;dll;exe;bat;pif;txt;xml;doc;bmp;mp3;mov;png;flp;"
    $path = "C:random1random2random3random4random5random6random7random8random9random10random11random12random13random14random15random16random17random18random19random20random21random22random23random24random25random26random27random28random29random30"
    $ext = "wanted_file.err" ;extension is not in the list, so both algorithms have to search everything through
    $path &= $ext

    ;Calculate the overhead for the loop and a lot of func calls
    $testtime = TimerInit()
    For $i = 0 To 0xFFFF
        testfunc($path, $types)
    Next
    $overhead = TimerDiff($testtime)

    ;test of older_IsValidType
    $starttime = TimerInit()
    For $i = 0 To 0xFFFF
        older_IsValidType($path, $types)
    Next
    $test1 = TimerDiff($starttime)

    ;test of old_IsValidType
    $starttime = TimerInit()
    For $i = 0 To 0xFFFF
        old_IsValidType($path, $types)
    Next
    $test2 = TimerDiff($starttime)

    ;test of _IsValidType
    $starttime = TimerInit()
    For $i = 0 To 0xFFFF
        _IsValidType($path, $types)
    Next
    $test3 = TimerDiff($starttime)

    ConsoleWrite("(scaled) Test data: " & @CRLF & _
            "Assumed overhead: " & $overhead & @CRLF & _
            "_isValidType: " & $test3 - $overhead & @CRLF & _
            "old_IsValidType: " & $test2 - $overhead & @CRLF & _
            "older_IsValidType: " & $test1 - $overhead & @CRLF)
EndFunc   ;==>ScaledTest

Func _IsValidType($sFilePath, $sList = "bat;cmd;exe")
    Return StringRegExp($sFilePath, ".(?i:Q" & StringReplace($sList, ";", "E|Q") & "E)z")
EndFunc   ;==>_IsValidType

Func old_IsValidType($sFilePath, $sList = "bat;cmd;exe")
    If StringRegExp($sList, "[/:<>|]") Then
        Return SetError(1, 0, -1)
    EndIf
    Return StringRegExp($sFilePath, "(?i)^(" & ".*?." & StringReplace($sList, ";", "|.*?.") & ")z")
EndFunc   ;==>old_IsValidType

Func older_IsValidType($path, $aftypes = "bat;cmd;exe")
    $etypes = StringSplit($aftypes, ";", 2)
    If Not IsArray($etypes) Or Not UBound($etypes) Then
        Local $types[3] = ["cmd", "bat", "exe"]
    Else
        Local $types = $etypes
    EndIf
    Local $ipos = StringInStr($path, ".", 0, -1), $stype
    If Not $ipos Then Return False
    $stype = StringMid($path, $ipos + 1)
    For $ext In $types
        If $ext = $stype Then
            Return True
        EndIf
    Next
    Return False
EndFunc   ;==>older_IsValidType

Func testfunc($a, $b)
EndFunc   ;==>testfunc

Edited February 1, 2012 by Robjong

[Beege]

A lot slower in the context, dude. Chill off.

CHILL? CHILL? My middle name cool!

But in all seriousness its not that I don't agree with some off the things your saying shaggi because I do. It was the way you presented it out. Guinness is a great coder with tons of his work laid all through out theses fourms and because of that lots people read, and learn from these threads. When they get to your first posts, they see you calling SRE slow, then some performance results making it to be 16x slow. They then take from that, WOW I'm never going to use SRE. So thats why it left a bad taste in my mouth.

edit: And the SRE guru Robjong has arrived. welcome

Edited February 1, 2012 by Beege

[Shaggi]

The timing test you provided has one major flaw, and the results should be disregarded, here is why.

The function _IsValidType handles the 'list' var ($sList) in the function every time it is called, the isvalidtype function uses the array created by StringSplit which is done once before the loop. If you modify the isvalidtype function to accept the same string as the second param and call StringSplit inside the function at every call the SRE method is faster.

Obviously, the interface is a bit different between the two functions. My functions loops an array for checks - the user provides the mask as an array, for performance of course, the other analyses a string for matches - the user provides a mask as a string as anything else would overly complicate the function (you would have to concate the array down to a string).

They are inherently not the same, and thus i dont think it would be fair to make input list in my function as a string, then convert it to an array (that would be idiotic), as it would be the other way around. Using an array for a list is preferred at all times in general, according to me. it doesn't make sense to keep a list in a string, and then split it everytime you need data from it imo, and therefore i dont think it disqualifies my function.

However, if you want to do that comparison, use 69255's function, as it looks more like the other (accepts a ; delimited string) - and is some 3-4 times faster than my original.

CHILL? CHILL? My middle name cool!

But in all seriousness its not that I don't agree with some off the things your saying shaggi because I do. It was the way you presented it out. Guinness is a great coder with tons of his work laid all through out theses fourms and because of that lots people read, and learn from these threads. When they get to your first posts, they see you calling SRE slow, then some performance results making it to be 16x slow. They then take from that, WOW I'm never going to use SRE. So thats why it left a bad taste in my mouth.

edit: And the SRE guru Robjong has arrived. welcome

Well i'm sorry, i obviously didn't know your middlename but when you start using bold text and swearwords (not that i dont though) defending something that is slower, well..

regexp is an advanced tool, and has many good uses, but i think that this example (derived from a recursive file search?) could benefit from added clearness and speed (why not?). And the topic invited for suggestions, so i posted one generally, the idom KISS (keep it simple, stupid!) should be followed at all times (i think, especially in public code / udf's).

[Robjong]

Obviously, the interface is a bit different between the two functions. My functions loops an array for checks - the user provides the mask as an array, for performance of course, the other analyses a string for matches - the user provides a mask as a string as anything else would overly complicate the function (you would have to concate the array down to a string).

Depends on the usage, if you need a different mask every time you will need to use multiple arrays, and for storing these values strings also come in handy, but point taken.

About the implementation of 69255, it is loads faster but this comes at a price, it will falsely match 'file.ex' if exe is in the list.

Edit: @Beege Hi

Edited February 1, 2012 by Robjong

[MvGulik]

Suggestion.

Slight change of the function name, as "_IsValidType" is very very general. (first think I though was 'function to check variable validity?)

Based on the description: "Checks if a filepath contains an approved filetype extension."

"_IsValidFileType" would be more explanatory when encountering this function inside some other code.

(or something like "_HasValidFileType" after a second read of the description. ... etc)

Edited February 1, 2012 by MvGulik

[Robjong]

@MvGulik, I had almost the same thought but I think _HasValidFileType could sound like a function to check the mime-type of a file, so I would like to suggest _HasValidFileExtension, minor detail.

btw. Im currently writing a validation UDF, should be ready one of these days.

Edit: phrasing

Edit 2: Changed _Is* to _Has because I agree with MvGulik's edit

Edited February 1, 2012 by Robjong

[guinness]

Hi Robjong,

You never cease to amaze me and I'm so glad you decided to drop on by. So having looked at your proposed solution in post #24 and tested it quite a bit, if you have no objection I've updated the original post with your solution and added all those who participated in this thread to the UDF header.

Guinness is a great coder with tons of his work laid all through out theses fourms

I beg to differ, you're not bad yourself.

[guinness]

Suggestion. Slight change of the function name, as "_IsValidType" is very very general. (first think I though was 'function to check variable validity?) Based on the description: "Checks if a filepath contains an approved filetype extension." "_IsValidFileType" would be more explanatory when encountering this function inside some other code. (or something like "_HasValidFileType" after a second read of the description. ... etc)

OK, I'll change the function name. Thanks.

Edit: I opted for _IsValidFileType just because it describes the function more, as MvGulik pointed out. I'm sure there was a reason behind me calling _IsValidType, but I don't remember.

Edited February 1, 2012 by guinness

[Robjong]

Hi Robjong,

You never cease to amaze me and I'm so glad you decided to drop on by. So having looked at your proposed solution in post #24 and tested it quite a bit, if you have no objection I've updated the original post with your solution and added all those who participated in this thread to the UDF header.

I have no objections whatsoever

[guinness]

I won't be using _Has for the same reason _IsPressed isn't called _HasBeenPressed.

[guinness]

I have no objections whatsoever

I thought you wouldn't but it's always polite to ask.

[Robjong]

I won't be using _Has for the same reason _IsPressed isn't called _HasBeenPressed.

Good point.

I thought you wouldn't but it's always polite to ask.

True, thank you for being polite.

About the function, the function header is a bit off, the Syntax is incorrect (optional brackets are misplaced) and it does have parameters, so this...

; Syntax.........: _IsValidType($sFilePath, [$sList])
;                 $sFilePath - File path of the file.
;                 $sList - [Optional] A list of filetype extensions separated with ';' e.g. "zip;rar;7zip;gzip"
; Parameters ....: None

... should be like this...

; Syntax.........: _IsValidFileType($sFilePath[, $sList])
; Parameters ....: $sFilePath - File path of the file.
;                 $sList - [Optional] A list of filetype extensions separated with ';' e.g. "zip;rar;7zip;gzip"

... right?

And I have a question. Why is the do you have the meta character check in there?

If StringRegExp($sList, "[/:<>|]") Then
    Return SetError(1, 0, -1)
EndIf

Edit: AutoIt tags

Edited February 1, 2012 by Robjong

[guinness]

You're correct, I created that header a while ago and to think I went through the help file correcting mistakes just like that.

And I have a question. Why is the do you have the meta character check in there?

I didn't realise I had to remove it, which I've now done. Thanks.

[Beege]

regexp is an advanced tool, and has many good uses, but i think that this example (derived from a recursive file search?) could benefit from added clearness and speed (why not?). And the topic invited for suggestions, so i posted one generally, the idom KISS (keep it simple, stupid!) should be followed at all times (i think, especially in public code / udf's).

Yes regexp can be advanced, but it certainly doesn't have to be. If you've never used it can defiantly understand how it can look pretty intimidating. But small simple examples like this one I feel are ideal candidates for showing off its uses and helping people learn. And as for the public code/ udf's, if you look there are quite a few of them using sre's in the autoit's standard udf collection.

[Beege]

You're correct, I created that header a while ago and to think I went through the help file correcting mistakes just like that.

I didn't realise I had to remove it, which I've now done. Thanks.

Ahhh I was just about to say that! Good thing your not in charge of are help file

[guinness]

Ahhh I was just about to say that! Good thing your not in charge of are help file

I like to imagine I am, but I know I'm not

if you look there are quite a few of them using sre's in the autoit's standard udf collection.

My favourite little SRE gem I've found in the UDFs was in _FileListToArray, I turned this into a little wrapper function too >>

[Beege]

My favourite little SRE gem I've found in the UDFs was in _FileListToArray, I turned this into a little wrapper function too >>

Dido. Thats a great little command I basically use with any function that I'm asking for a directory. Nice little wrapper you made there too.

[guinness]

In WinAPIEx it's either _WinAPI_PathAddBackslash or _WinAPI_PathRemoveBackslash depending on what you want to do.