Improvement of included _FileListToArray function.

[Wus]

A 27 line function was just turned into a 186 line function.

What's wrong with that?

I can only speak for myself and from my personal experience of managing a moderately sized project for ~6 months (I only mention this so you won't dismiss my opinion as uninformed), but if if one of the guys I'd worked with had said that, well, I would have slapped them with a large trout. There is always one more tweak that can make your code faster -- almost always at the expense of clarity and brevity -- and the question is 'Where do I draw the line?', which is only gained through experience. Start doing some real debugging and I believe you will quickly appreciate my sentiments.

[jpm]

I can only speak for myself and from my personal experience of managing a moderately sized project for ~6 months (I only mention this so you won't dismiss my opinion as uninformed), but if if one of the guys I'd worked with had said that, well, I would have slapped them with a large trout. There is always one more tweak that can make your code faster -- almost always at the expense of clarity and brevity -- and the question is 'Where do I draw the line?', which is only gained through experience. Start doing some real debugging and I believe you will quickly appreciate my sentiments.

I always respect addition that people request unless it was out of the scope .

The exclude and the recursion is a good think.

The function was cleverly written for the speed (50%) improvement.

The clarity was well handled. The switch well describe when the specific sequence is used.

I am sorry for those which have given a lot of effort which are not included in the next beta

[Zedna]

Anyway, I have an idea on how to include this without violating the points I made previously on why it shouldn't be added.

Original idea was to have untouched _FileListToArray() as it is now

and add new improved version from this topic as _FileListToArrayEx() or some similar name.

This way there is no problem you are talking about.

[Spiff59]

I'm sorry, but I am failing to understand this whole dilemma. AutoIT has a _FileListToArray function that works fine. I've used it and have been quite happy with the results. If the built-in version is not fast enough for everyone then they can search the forum and find this version. Voila! Done. Everyone gets what they want.

All you need do is search the forums and see the number of topics created rgarding FLTA's shortcomings. Look at the number of modified versions floating around in the wild. Look at the number of BugTracs created. Not many others are overly impressed with the current FLTA.

On a side note, the need for speed is a dangerous one for programmers. Trying to optimize a function generally results in:

• More bugs that were completely unforeseen
• More overall complexity in the input and output arguments
• Much more complexity in the code
• An enormous amount of time wasted that could have been better used

I'm sorry you've had such bad luck.

We're not working on a Honeywell CP-V like I learned on that had 32MB of core, or an IBM 4331 like at my first job that was maxed at 64MB. I find stressing over a couple hundred bytes as a reason to shoot down a vast improvement to a frequently-used and time-consuming function baseless. Especially when so many in the community have called for such an enhancement.

Playing the "What if it's broken?" fear card is also a poor reason to impede progress. This version is structured, well documented, efficient and flexible. It's written in a straight-forward manner that is hardly complex. It contains features found in most similar search functions or other languages/utilities.

The existing FLTA is demonstrably flawed. It is broken. It is unstable. This version is much faster, very flexible, and, so far, without defect. I agree that more testing is prudent. Other reasons for not replacing the status quo with an all-around better routine dont hold much water.

[Wus]

It might be the case that I spoke a bit hastily in regards to this function, and I shall educate myself a bit more before posting anything more on it. Having said that, my comments on what I called 'a need for speed' I resolutely stand behind.

[Valik]

Wus is 100% right in everything he has said. It's rather disappointing that only like two of you are actually smart enough to understand how programming works and the rest of you are a bunch of talking heads who don't have a clue and who think just because something is written it must be useful.

There are far too many replies with stupidity in them to respond to all of them. Instead I'm going to reply to the last three.

I always respect addition that people request unless it was out of the scope.

Yes, you always respect their additions. What you don't respect is me or my experience doing this which comes from my experience with other languages.

Original idea was to have untouched _FileListToArray() as it is now

and add new improved version from this topic as _FileListToArrayEx() or some similar name.

This way there is no problem you are talking about.

My idea is similar to that.

All you need do is search the forums and see the number of topics created rgarding FLTA's shortcomings. Look at the number of modified versions floating around in the wild. Look at the number of BugTracs created. Not many others are overly impressed with the current FLTA.

This sounds like a reason to remove the function to me. User-dissatisfaction and a constantly changing code-base.

We're not working on a Honeywell CP-V like I learned on that had 32MB of core, or an IBM 4331 like at my first job that was maxed at 64MB. I find stressing over a couple hundred bytes as a reason to shoot down a vast improvement to a frequently-used and time-consuming function baseless. Especially when so many in the community have called for such an enhancement.

This is an exceptionally stupid statement. We aren't managing a few thousand lines of code here. We're managing tens of thousands of lines of C++ and AutoIt code. When you make a function 7x longer and more complex in the most absurd of ways you're opening a can of worms that will never end. As was previously mentioned, I can find an optimization to every single function. Where do I draw the line? I bet I can find ways to optimize your revised function if I cared to take a look at it. So should we optimize every single one of our hundreds of functions to maximize speed?

It's written in a straight-forward manner that is hardly complex.

Bullshit. It's written in a very stupid manner that just happens to be fast in this particular language.

It contains features found in most similar search functions or other languages/utilities.

But it is not written like a function would be in other languages. I bet I could optimize the crap out of functions in other languages, too. But other languages take a more practical approach by writing something that is stable and a bit slower and a heck of a lot less complex. Because those are the design criteria for a library, not "wickedly fast".

The existing FLTA is demonstrably flawed. It is broken. It is unstable. This version is much faster, very flexible, and, so far, without defect. I agree that more testing is prudent. Other reasons for not replacing the status quo with an all-around better routine dont hold much water.

Last sentence is made out of ignorance. It's basically stupid-user syndrome. Writing code for a library is not the same as writing code for personal use. It seems like only a couple people actually understand that concept. My goals as a language developer are far different than the more self-centered user-centric goals. You need to accept that and spare me the stupid arguments.

I'm going to give you all a piece of advice: Shut the fuck up. I'll see about including this function in some fashion in the next beta. But if I continue to see an outpouring of stupidity and ignorance from the community the end result is not only going to be never seeing this function but also the removal of the currently _FileListToArray(). Functions that cause so much drama do not belong in the language regardless of their merits.

So just cool it.

[Malkey]

When using this script with the function from Post #175, the results are displaying "1's" randomly attached to the end of some file names.

;
      #include <Array.au3>
      
      Local $aArr = _FileListToArrayNT7("C:\Program Files\AutoIt3", "*Get*.au3", 1, 2, True, "", 1)
      
      _ArrayDisplay($aArr)
    ;

Edit: Added "some" file names.

Edited July 6, 2009 by Malkey

[Wus]

By the way, I really don't like the fact that you have an argument that is not supposed to be changed.

$sWorkPath =   *** internal use only (do not use) ***

Could you not implement this method without the recursion? Say with a stack or a queue? I am guessing a DFS, i.e. queue, would work pretty well.

[Valik]

By the way, I really don't like the fact that you have an argument that is not supposed to be changed.

$sWorkPath =   *** internal use only (do not use) ***

Could you not implement this method without the recursion? Say with a stack or a queue? I am guessing a DFS, i.e. queue, would work pretty well.

It uses recursion? Wow, fail. That right there is reason enough for me not to include it.

[Spiff59]

When using this script with the function from Post #175, the results are displaying "1's" randomly attached to the end of some file names.

;
      #include <Array.au3>
      
      Local $aArr = _FileListToArrayNT7("C:\Program Files\AutoIt3", "*Get*.au3", 1, 2, True, "", 1)
      
      _ArrayDisplay($aArr)
;

Edit: Added "some" file names.

That's interesting. It is a result of the very last change that I had championed just yesterday.

It was a change that had had some discussion here.

Reverting "Return SetError(4)" to "Return SetError(4, 4, "")" cures the issue.

I had thought setting the @extended information unnecessary.

It appears it is necessary to force the "" override of the return value.

Why in most cases "Return SetError(4)" returns nothing, and sometimes a "1" is not a behavior I can explain.

Edit: Sorry, BaKaMu. Apparently a bad idea. Better put the SetErrors back the way they were.

Edited July 6, 2009 by Spiff59

[Valik]

Why in most cases "Return SetError(4)" returns nothing, and sometimes a "1" is not a behavior I can explain.

There is no defined return value for SetError() unless the user specifies one. The current default return value for functions without a defined return value is 1.

[BaKaMu]

Edit: Sorry, BaKaMu. Apparently a bad idea. Better put the SetErrors back the way they were.

Done (Post #175), i'm a little bit amused ;-)

[Valik]

i'm a little bit amused ;-)

Me too.

[Spiff59]

It's clear that if the Post #175 version ever sees the light of day it will be as an "Ex" version.

This thread originated with the idea of a simple replacement and a few pages later grew, by approval of the thread majority, into the all-the-bells-and-whistles version.

The 3.3.0.0 production version (and Beta 3.3.1.1) are documented to return "@error = 4" when no matching files or folders are found. It is not working as described/intended. Only when $iFlag = 0 does the function return the @error condition.

What chance might the second function in the test script below have of success on the merit of being a *bug fix* for the above-stated problem? It's 32 lines, total, and corrects the bug. (Somehow it accidentally got nearly twice as fast as the 3.3.1.1 version. I'm not sure how that happened).

expandcollapse popup

;include<array.au3>
$Path = @SystemDir
;$Path = "C:\Program Files\Autoit3\"
$Flag = 0 ; 0 = files/folder, 1 = files only, 2 = folders only
$repeat = 100

; Beta version 3.3.1.1 with 1-line @error bug fix ---------------------------
$timer = TimerInit()
For $j = 1 to $repeat
    $x = _FileListToArray1($Path, "*", $Flag)
Next
$t1 = TimerDiff ($timer)

; Modified Beta version with @error bug fix ---------------------------------
$timer = TimerInit()
For $j = 1 to $repeat
    $x = _FileListToArray2($Path, "*", $Flag)
Next
$t2 = TimerDiff ($timer)

MsgBox (0, "", "$iFlag = " & $Flag & @CRLF &"Ver1:  " & Int($t1/100)/10 & @CRLF & "Ver1:  " & Int($t2/100)/10)
_ArrayDisplay($x)

;===============================================================================
Func _FileListToArray1($sPath, $sFilter = "*", $iFlag = 0) ; version from Beta 3.3.1.1  with fix - 28 lines
    Local $hSearch, $sFile, $asFileList[1]
    If Not FileExists($sPath) Then Return SetError(1, 1, "")
    If (StringInStr($sFilter, "\")) Or (StringInStr($sFilter, "/")) Or (StringInStr($sFilter, ":")) Or (StringInStr($sFilter, ">")) Or (StringInStr($sFilter, "<")) Or (StringInStr($sFilter, "|")) Or (StringStripWS($sFilter, 8) = "") Then Return SetError(2, 2, "")
    If Not ($iFlag = 0 Or $iFlag = 1 Or $iFlag = 2) Then Return SetError(3, 3, "")
    If (StringMid($sPath, StringLen($sPath), 1) = "\") Then $sPath = StringTrimRight($sPath, 1) ; needed for Win98 for x:\  root dir
    $hSearch = FileFindFirstFile($sPath & "\" & $sFilter)
    If $hSearch = -1 Then Return SetError(4, 4, "")
    While 1
        $sFile = FileFindNextFile($hSearch)
        If @error Then
            SetError(0)
            ExitLoop
        EndIf
        If $iFlag = 1 And @extended Then
            ContinueLoop        ; File only
        Else
            If $iFlag = 2 And @extended = 0 Then ContinueLoop   ; folder only
        EndIf
        $asFileList[0] += 1
        If UBound($asFileList) <= $asFileList[0] Then ReDim $asFileList[UBound($asFileList) * 2]
        $asFileList[$asFileList[0]] = $sFile
    WEnd
    FileClose($hSearch)
    If Not $asFileList[0] Then Return SetError(4, 4, "") ; added to 3.3.1.1
    ReDim $asFileList[$asFileList[0] + 1] ; Trim unused slots
    Return $asFileList
EndFunc   ;==>_FileListToArray

;===============================================================================
Func _FileListToArray2($sPath, $sFilter = "*", $iFlag = 0) ; modified beta with bug fix - 32 lines
Local $hSearch, $sFile, $sFileList, $sDelim = "|"
If Not FileExists($sPath) Then Return SetError(1, 1, "")
If StringRegExp($sFilter, "[\\/:<>|]") Or (Not StringStripWS($sFilter, 8)) Then Return SetError(2, 2, "")
If Not ($iFlag = 0 Or $iFlag = 1 Or $iFlag = 2) Then Return SetError(3, 3, "")
If StringRight($sPath, 1) <> "\" Then $sPath &= "\"
$hSearch = FileFindFirstFile($sPath & $sFilter)
If @error Then Return SetError(4, 4, "")
Switch $iFlag
    Case 0; Files and Folders
        While 1
            $sFile = FileFindNextFile($hSearch)
            If @error Then ExitLoop
            $sFileList &= $sDelim & $sFile
        WEnd
    Case 1; Files Only
        While 1
            $sFile = FileFindNextFile($hSearch)
            If @error Then ExitLoop
            If @extended = 0 Then $sFileList &= $sDelim & $sFile
        WEnd
    Case 2; Folders Only
        While 1
            $sFile = FileFindNextFile($hSearch)
            If @error Then ExitLoop
            If @extended Then $sFileList &= $sDelim & $sFile
        WEnd
EndSwitch
FileClose($hSearch)
If Not $sFileList Then Return SetError(4, 4, "")
Return StringSplit(StringTrimLeft($sFileList, 1), "|")
EndFunc;==>_FileListToArray

Edit: And yes, one line could fix the existing 3.3.1.1 function, making it 28 lines. So, the 40% performance increase above is at the cost of an additional 4 lines of code. (I went back and added the one-line fix to the first function above for a more accurate benchmark). Anyway, the bug ought to be addressed.

Edited July 6, 2009 by Spiff59

[Wus]

Just to elaborate on what I was saying earlier about not needing to do actual recursion, I wrote the following code.

expandcollapse popup

#include <Array.au3>

Dim $results = _FLTA_NonRecursiveRecursion("C:\Program Files\AutoIt3\", True)
_ArrayDisplay($results)

Func _FLTA_NonRecursiveRecursion($sPath, $bRecursive)
    ; Make a stack
    Local $iStackCapacity = 64
    Local $iStackSize = 0
    Local $aFolderStack[$iStackCapacity]
    ; Ready the return variable
    Local $sRet = ""
    ; Seed the stack with the start directory
    $aFolderStack[0] = $sPath
    $iStackSize += 1
    ; Loop while there is a directory left to be explored
    While $iStackSize > 0
        ; Change to the new directory you want to explore
        FileChangeDir($aFolderStack[$iStackSize-1])
        $iStackSize -= 1
        ; Initiate directory listing
        Local $search = FileFindFirstFile("*")
        Local $file = ""
        ; Loops until current directory is completely explored
        While 1
            ; Grab the next item in the directory
            $file = FileFindNextFile($search)
            ; Kick out if listing is exhausted
            If @error Then ExitLoop
            ; Decide if item is File or Folder
            If StringInStr(FileGetAttrib($file), "D") Then
                ; It is a folder.  Add to queue if listing is recursive.
                If $bRecursive Then
                    ; Redim stack if needed.
                    If $iStackCapacity <= $iStackSize Then
                        $iStackCapacity *= 2
                        ReDim $aFolderStack[$iStackCapacity]
                    EndIf
                    ; Add folder to stack
                    $aFolderStack[$iStackSize] = @WorkingDir & "\" & $file
                    $iStackSize += 1
                EndIf
            Else
                ; It is a file.  Add to listing.
                $sRet &= "|" & @WorkingDir & "\" & $file
            EndIf
        WEnd
    WEnd
    ; Return an array version of the total listing
    Return StringSplit($sRet, "|")
EndFunc   ;==>_FLTA_NonRecursiveRecursion

It lists every file in a directory recursively (or not depending on the boolean flag) without any actual recursion. It is only supposed to be a proof of concept, and is not intended to be anything more. I obviously didn't try to do any error checking or include all those fancy schmancy speedups or add all the functionality. I used a stack, so it is just a depth-first search. It ought to work with or without the beta.

EDIT:

Typo.

Edited July 6, 2009 by Wus

[MrCreatoR]

Just to elaborate on what I was saying earlier about not needing to do actual recursion, I wrote the following code.

It's strangly reminding my code for DirGetSizeEx function - And this algorithm i also have taken from someone else (amel27 if i remember correctly).

But sure, this is how recursion should be replaced.

[Wus]

It's strangly reminding my code for DirGetSizeEx function - And this algorithm i also have taken from someone else (amel27 if i remember correctly).

I'm not entirely sure what the means, but I can assure you I just wrote that function 100% from scratch without ever having seen your code or amel27's code in my life. If they look very similar (which I agree they do) it is because they both use the same extremely well-known algorithm, and we both used common sense. I'm sorry if I sound a bit touchy on the subject, but I am going to grad school for what I hope to be a life long career of research and my intellectual integrity is not something I take lightly. If you want further proof, then I can send you a few dozen java projects I have which use the same basic code template in a variety of circumstances, thus showing that the code is by no means unique in its nature. Edited July 6, 2009 by Wus

[Valik]

Indeed. I have written - and posted - a stack-based file listing function long before any of those users posted anything here. It's pretty much how it's done although the growth factor is... not what I would use (I grow by 1.5, not 2).

[MrCreatoR]

I'm not entirely sure what the means, but I can assure you I just wrote that function 100% from scratch without ever having seen your code or amel27's code in my life. If they look very similar (which I agree they do) it is because they both use the same extremely well-known algorithm, and we both used common sense. I'm sorry if I sound a bit touchy on the subject, but I am going to grad school for what I hope to be a life long career of research and my intellectual integrity is not something I take lightly. If you want further proof, then I can send you a few dozen java projects I have which use the same basic code template in a variety of circumstances, thus showing that the code is by no means unique in its nature.

You don't have to proof anything, i am honestly believe you. It's just was strange to see it, that's all

P.S

Good luck with the graduation

[Zedna]

@Wus

Very nice educative example. Thanks for sharing.

This nonrecursive recursion concept may be very useful generally.