Wus Posted July 5, 2009 Share Posted July 5, 2009 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. Link to comment Share on other sites More sharing options...
jpm Posted July 5, 2009 Share Posted July 5, 2009 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 Link to comment Share on other sites More sharing options...
Zedna Posted July 5, 2009 Share Posted July 5, 2009 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 nowand add new improved version from this topic as _FileListToArrayEx() or some similar name.This way there is no problem you are talking about. Resources UDF ResourcesEx UDF AutoIt Forum Search Link to comment Share on other sites More sharing options...
Spiff59 Posted July 5, 2009 Share Posted July 5, 2009 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 unforeseenMore overall complexity in the input and output argumentsMuch more complexity in the codeAn enormous amount of time wasted that could have been better usedI'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. Link to comment Share on other sites More sharing options...
Wus Posted July 5, 2009 Share Posted July 5, 2009 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. Link to comment Share on other sites More sharing options...
Valik Posted July 5, 2009 Share Posted July 5, 2009 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 nowand 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. Link to comment Share on other sites More sharing options...
Malkey Posted July 6, 2009 Share Posted July 6, 2009 (edited) 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 Link to comment Share on other sites More sharing options...
Wus Posted July 6, 2009 Share Posted July 6, 2009 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. Link to comment Share on other sites More sharing options...
Valik Posted July 6, 2009 Share Posted July 6, 2009 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. Link to comment Share on other sites More sharing options...
Spiff59 Posted July 6, 2009 Share Posted July 6, 2009 (edited) 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 Link to comment Share on other sites More sharing options...
Valik Posted July 6, 2009 Share Posted July 6, 2009 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. Link to comment Share on other sites More sharing options...
BaKaMu Posted July 6, 2009 Share Posted July 6, 2009 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 ;-) Link to comment Share on other sites More sharing options...
Valik Posted July 6, 2009 Share Posted July 6, 2009 i'm a little bit amused ;-)Me too. Link to comment Share on other sites More sharing options...
Spiff59 Posted July 6, 2009 Share Posted July 6, 2009 (edited) 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 Link to comment Share on other sites More sharing options...
Wus Posted July 6, 2009 Share Posted July 6, 2009 (edited) 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 Link to comment Share on other sites More sharing options...
MrCreatoR Posted July 6, 2009 Share Posted July 6, 2009 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. Spoiler Using OS: Win 7 Professional, Using AutoIt Ver(s): 3.3.6.1 / 3.3.8.1 AutoIt Russian Community My Work... Spoiler Projects: ATT - Application Translate Tool {new}| BlockIt - Block files & folders {new}| SIP - Selected Image Preview {new}| SISCABMAN - SciTE Abbreviations Manager {new}| AutoIt Path Switcher | AutoIt Menu for Opera! | YouTube Download Center! | Desktop Icons Restorator | Math Tasks | KeyBoard & Mouse Cleaner | CaptureIt - Capture Images Utility | CheckFileSize ProgramUDFs: OnAutoItErrorRegister - Handle AutoIt critical errors {new}| AutoIt Syntax Highlight {new}| Opera Library! | Winamp Library | GetFolderToMenu | Custom_InputBox()! | _FileRun UDF | _CheckInput() UDF | _GUIInputSetOnlyNumbers() UDF | _FileGetValidName() UDF | _GUICtrlCreateRadioCBox UDF | _GuiCreateGrid() | _PathSplitByRegExp() | _GUICtrlListView_MoveItems - UDF | GUICtrlSetOnHover_UDF! | _ControlTab UDF! | _MouseSetOnEvent() UDF! | _ProcessListEx - UDF | GUICtrl_SetResizing - UDF! | Mod. for _IniString UDFs | _StringStripChars UDF | _ColorIsDarkShade UDF | _ColorConvertValue UDF | _GUICtrlTab_CoverBackground | CUI_App_UDF | _IncludeScripts UDF | _AutoIt3ExecuteCode | _DragList UDF | Mod. for _ListView_Progress | _ListView_SysLink | _GenerateRandomNumbers | _BlockInputEx | _IsPressedEx | OnAutoItExit Handler | _GUICtrlCreateTFLabel UDF | WinControlSetEvent UDF | Mod. for _DirGetSizeEx UDF Examples: ScreenSaver Demo - Matrix included | Gui Drag Without pause the script | _WinAttach()! | Turn Off/On Monitor | ComboBox Handler Example | Mod. for "Thinking Box" | Cool "About" Box | TasksBar Imitation Demo Like the Projects/UDFs/Examples? Please rate the topic (up-right corner of the post header: Rating ) * === My topics === * ================================================== ================================================== AutoIt is simple, subtle, elegant. © AutoIt Team Link to comment Share on other sites More sharing options...
Wus Posted July 6, 2009 Share Posted July 6, 2009 (edited) 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 Link to comment Share on other sites More sharing options...
Valik Posted July 6, 2009 Share Posted July 6, 2009 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). Link to comment Share on other sites More sharing options...
MrCreatoR Posted July 6, 2009 Share Posted July 6, 2009 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.SGood luck with the graduation Spoiler Using OS: Win 7 Professional, Using AutoIt Ver(s): 3.3.6.1 / 3.3.8.1 AutoIt Russian Community My Work... Spoiler Projects: ATT - Application Translate Tool {new}| BlockIt - Block files & folders {new}| SIP - Selected Image Preview {new}| SISCABMAN - SciTE Abbreviations Manager {new}| AutoIt Path Switcher | AutoIt Menu for Opera! | YouTube Download Center! | Desktop Icons Restorator | Math Tasks | KeyBoard & Mouse Cleaner | CaptureIt - Capture Images Utility | CheckFileSize ProgramUDFs: OnAutoItErrorRegister - Handle AutoIt critical errors {new}| AutoIt Syntax Highlight {new}| Opera Library! | Winamp Library | GetFolderToMenu | Custom_InputBox()! | _FileRun UDF | _CheckInput() UDF | _GUIInputSetOnlyNumbers() UDF | _FileGetValidName() UDF | _GUICtrlCreateRadioCBox UDF | _GuiCreateGrid() | _PathSplitByRegExp() | _GUICtrlListView_MoveItems - UDF | GUICtrlSetOnHover_UDF! | _ControlTab UDF! | _MouseSetOnEvent() UDF! | _ProcessListEx - UDF | GUICtrl_SetResizing - UDF! | Mod. for _IniString UDFs | _StringStripChars UDF | _ColorIsDarkShade UDF | _ColorConvertValue UDF | _GUICtrlTab_CoverBackground | CUI_App_UDF | _IncludeScripts UDF | _AutoIt3ExecuteCode | _DragList UDF | Mod. for _ListView_Progress | _ListView_SysLink | _GenerateRandomNumbers | _BlockInputEx | _IsPressedEx | OnAutoItExit Handler | _GUICtrlCreateTFLabel UDF | WinControlSetEvent UDF | Mod. for _DirGetSizeEx UDF Examples: ScreenSaver Demo - Matrix included | Gui Drag Without pause the script | _WinAttach()! | Turn Off/On Monitor | ComboBox Handler Example | Mod. for "Thinking Box" | Cool "About" Box | TasksBar Imitation Demo Like the Projects/UDFs/Examples? Please rate the topic (up-right corner of the post header: Rating ) * === My topics === * ================================================== ================================================== AutoIt is simple, subtle, elegant. © AutoIt Team Link to comment Share on other sites More sharing options...
Zedna Posted July 6, 2009 Share Posted July 6, 2009 @Wus Very nice educative example. Thanks for sharing. This nonrecursive recursion concept may be very useful generally. Resources UDF ResourcesEx UDF AutoIt Forum Search Link to comment Share on other sites More sharing options...
Recommended Posts
Create an account or sign in to comment
You need to be a member in order to leave a comment
Create an account
Sign up for a new account in our community. It's easy!
Register a new accountSign in
Already have an account? Sign in here.
Sign In Now