BestCodingPractice Analyzer ;)

[mLipok]

Erroneous declarations are really just the tip of the iceberg. People don't always agree on what is, or isn't, good coding practice. I'm not dismissing your idea because it may have potential in terms of teaching AutoIt basics.

 

I also see the logic of your example: I can't think of any reason why someone would want to decalre a variable* within a loop other than to test the interpreter in some way. A  distinction should be made between that which is clearly wrong and that which is mostly a matter of opinion.

 

* the same variable

 

 

EDIT: I do not understand. 

I once alone did not comply with the rules (BCP).

Everybody is not complied with.

See yourself in my script option to check directories.

select the directory Beta ... INCLUDE .. and you see that:

You wrong interpret this subject.

I myself sometime not so long ago, not behaved these principles (Best Coding Practice)

I have a lot of old scripts.

Everyone probably has a lot of them.

 

Try to check my program files directory INCLUDE

eg AutoIt 3.3.9.23 BETA

END OF EDIT.

expandcollapse popup

Quote
======================================================
C:\Program Files (x86)\AutoIt3\Beta\Include\Array.au3
======================================================
While 1

Local $aiCurItems[1] = [0]

Local $aiCurItems[1] = [0]

------------------------------------------------------
======================================================
C:\Program Files (x86)\AutoIt3\Beta\Include\EventLog.au3
======================================================
For $iI = 1 To $aMsgDLL[0]

Local $hDLL = _WinAPI_LoadLibraryEx($aMsgDLL[$iI], $__EVENTLOG_LOAD_LIBRARY_AS_DATAFILE)
Local $tBuffer = DllStructCreate("wchar Text[4096]")

Local $tBuffer = DllStructCreate("wchar Text[4096]")

------------------------------------------------------
======================================================
C:\Program Files (x86)\AutoIt3\Beta\Include\File.au3
======================================================
For $i = 1 To 2

Local $nServerLen = StringInStr($sFullPath, "\") - 1

Local $nServerLen = StringInStr($sFullPath, "\") - 1

------------------------------------------------------
======================================================
C:\Program Files (x86)\AutoIt3\Beta\Include\GuiListView.au3
======================================================
For $i = 0 To $items - 1

Local $a_indices[2]

Local $a_indices[2]

------------------------------------------------------
======================================================
C:\Program Files (x86)\AutoIt3\Beta\Include\GuiTreeView.au3
======================================================
While $hItem <> 0x00000000

Local $h_child = _SendMessage($hWnd, $TVM_GETNEXTITEM, $TVGN_CHILD, $hItem, 0, "wparam", "handle")

Local $h_child = _SendMessage($hWnd, $TVM_GETNEXTITEM, $TVGN_CHILD, $hItem, 0, "wparam", "handle")


While $hStart <> 0x00000000

Local $sItem = _GUICtrlTreeView_GetText($hWnd, $hStart)

Local $sItem = _GUICtrlTreeView_GetText($hWnd, $hStart)

------------------------------------------------------
======================================================
C:\Program Files (x86)\AutoIt3\Beta\Include\IE.au3
======================================================
For $o_window In $o_ShellWindows

Local $f_found = False

Local $f_found = False

------------------------------------------------------
======================================================
C:\Program Files (x86)\AutoIt3\Beta\Include\Process.au3
======================================================
Do ; Pseudo loop

Local $aProcessHandle = DllCall($hDLL, 'handle', 'OpenProcess', 'dword', $PROCESS_QUERY_INFORMATION, 'bool', False, 'dword', $iPID)
Local $aPriority = DllCall($hDLL, 'dword', 'GetPriorityClass', 'handle', $aProcessHandle[0])

Local $aPriority = DllCall($hDLL, 'dword', 'GetPriorityClass', 'handle', $aProcessHandle[0])

------------------------------------------------------
======================================================
C:\Program Files (x86)\AutoIt3\Beta\Include\SQLite.au3
======================================================
For $i = 0 To $iColumns - 1

Local $iRval_coltype = DllCall($g_hDll_SQLite, "int:cdecl", "sqlite3_column_type", "ptr", $hQuery, "int", $i)
Local $sRval = DllCall($g_hDll_SQLite, "wstr:cdecl", "sqlite3_column_text16", "ptr", $hQuery, "int", $i)
Local $vResult = DllCall($g_hDll_SQLite, "ptr:cdecl", "sqlite3_column_blob", "ptr", $hQuery, "int", $i)
Local $iColBytes = DllCall($g_hDll_SQLite, "int:cdecl", "sqlite3_column_bytes", "ptr", $hQuery, "int", $i)
Local $vResultStruct = DllStructCreate("byte[" & $iColBytes[0] & "]", $vResult[0])

Local $vResultStruct = DllStructCreate("byte[" & $iColBytes[0] & "]", $vResult[0])

------------------------------------------------------
======================================================
C:\Program Files (x86)\AutoIt3\Beta\Include\String.au3
======================================================
For $i_EncryptCountF = 0 To $i_EncryptLevel Step 1

Local $av_EncryptBox[256][2]

Local $av_EncryptBox[256][2]


For $i_EncryptCountF = 0 To $i_EncryptLevel Step 1

Local $av_EncryptBox[256][2]

Local $av_EncryptBox[256][2]

------------------------------------------------------
======================================================
C:\Program Files (x86)\AutoIt3\Beta\Include\WinAPIDiag.au3
======================================================
Do

Local $Ret = DllCall('dbghelp.dll', 'dword', 'SymGetOptions')
Local $hEnumProc = DllCallbackRegister('__EnumSymbolsProcA', 'int', 'ptr;ulong;lparam')
Local $pEnumProc = DllCallbackGetPtr($hEnumProc)

Local $pEnumProc = DllCallbackGetPtr($hEnumProc)


Do

Local $Ret = DllCall('dbghelp.dll', 'dword', 'SymGetOptions')
Local $hEnumProc = DllCallbackRegister('__EnumSymbolsProcW', 'int', 'ptr;ulong;lparam')
Local $pEnumProc = DllCallbackGetPtr($hEnumProc)

Local $pEnumProc = DllCallbackGetPtr($hEnumProc)


Do

Local $tData = DllStructCreate('byte[' & BinaryLen($sData) & ']')
Local $Ret = DllCall('advapi32.dll', 'int', 'CryptHashData', 'ptr', $hHash, 'struct*', $tData, _

Local $Ret = DllCall('advapi32.dll', 'int', 'CryptHashData', 'ptr', $hHash, 'struct*', $tData, _

For $i = 1 To $aData[0]

Local $Offset = Number(DllStructGetPtr($tData, $Count) - $pData)
Local $iSize
Local $ID, $Init
Local $Pattern = '[%0' & StringLen($Index) & 'd] '

Local $Pattern = '[%0' & StringLen($Index) & 'd] '

------------------------------------------------------
======================================================
C:\Program Files (x86)\AutoIt3\Beta\Include\WinAPIFiles.au3
======================================================
Do

Local $Ret = DllCall('kernel32.dll', 'bool', 'ReadFile', 'handle', $hFile, 'ptr', $pData, 'dword', 2, 'dword*', 0, 'ptr', 0)

Local $Ret = DllCall('kernel32.dll', 'bool', 'ReadFile', 'handle', $hFile, 'ptr', $pData, 'dword', 2, 'dword*', 0, 'ptr', 0)

------------------------------------------------------
======================================================
C:\Program Files (x86)\AutoIt3\Beta\Include\WinAPIGdi.au3
======================================================
While $Count > $Index

Local $Size[2]
Local $tData = DllStructCreate('byte Width;byte Height;byte ColorCount;byte Reserved;ushort Planes;ushort BitCount;long Size;long Offset', $pIco + 6 + 16 * $Index)

Local $tData = DllStructCreate('byte Width;byte Height;byte ColorCount;byte Reserved;ushort Planes;ushort BitCount;long Size;long Offset', $pIco + 6 + 16 * $Index)

Do

Local Const $tagIMAGECODECINFO = 'byte[16] Clsid;byte[16] FormatID;ptr szCodecName;ptr szDllName;ptr szFormatDescription;ptr szFilenameExtension;ptr szMimeType;dword Flags;dword Version;dword SigCount;dword SigSize;ptr pbSigPattern;ptr pbSigMask'
Local Const $tagENCODERPARAMETER = 'byte[16] GUID;ulong NumberOfValues;dword Type;ptr pValue'

Local Const $tagENCODERPARAMETER = 'byte[16] GUID;ulong NumberOfValues;dword Type;ptr pValue'


Do

Local $tICONINFO = DllStructCreate($tagICONINFO)
Local $Ret = DllCall('user32.dll', 'bool', 'GetIconInfo', 'handle', $hTemp, 'struct*', $tICONINFO)
Local $tBITMAP = DllStructCreate($tagBITMAP)
Local $hDC = _WinAPI_CreateCompatibleDC(0)
Local $hSv = _WinAPI_SelectObject($hDC, $hBitmap)

Local $hSv = _WinAPI_SelectObject($hDC, $hBitmap)


Do

Local $tDIB[2]
Local $Ret = DllCall('user32.dll', 'lresult', 'CallWindowProc', 'ptr', __ANDProc(), 'ptr', 0, 'uint', 0, _

Local $Ret = DllCall('user32.dll', 'lresult', 'CallWindowProc', 'ptr', __ANDProc(), 'ptr', 0, 'uint', 0, _


Do

Local $tDIB = DllStructCreate($tagDIBSECTION)

Local $tDIB = DllStructCreate($tagDIBSECTION)


Do

Local $Data[4][2]
Local $tTable = 0
Local $hFile = _WinAPI_CreateFile($sFile, 1, 4)
Local $Bytes

Local $Bytes


Do

Local $Bytes
Local $Info[8], $Ret, $pData = 0, $Index = 0

Local $Info[8], $Ret, $pData = 0, $Index = 0

------------------------------------------------------
======================================================
C:\Program Files (x86)\AutoIt3\Beta\Include\WinAPIProc.au3
======================================================
Do

Local $tPtr = DllStructCreate('ptr[' & $Count & ']')

Local $tPtr = DllStructCreate('ptr[' & $Count & ']')

------------------------------------------------------
======================================================
C:\Program Files (x86)\AutoIt3\Beta\Include\WinAPIRes.au3
======================================================
Do

Local $pData = __ResLoad($hModule, $iType, $iName, $iLanguage)
Local $tData = DllStructCreate('ushort;ushort', $Ret[3])

Local $tData = DllStructCreate('ushort;ushort', $Ret[3])

------------------------------------------------------
======================================================
C:\Program Files (x86)\AutoIt3\Beta\Include\WinAPISys.au3
======================================================
For $i = 0 To $Count - 1

Local $Dir = _WinAPI_PathIsDirectory($Ret[3])

Local $Dir = _WinAPI_PathIsDirectory($Ret[3])

------------------------------------------------------

Edited December 4, 2013 by mlipok

[mLipok]

I edit previous post.

I hope now you see the usefulness of the proposed solution.

[czardas]

You wrong interpret this subject.

 

Hmm, maybe I didn't understand you. It seems to me that you are just focussing on local variables declared within loops. I would consider this to be worse than bad practice because it produces repetition of redundant commands. I would classify this as a non-fatal error.

Edited December 4, 2013 by czardas

[BrewManNH]

It's not an error to constantly redeclare a variable like that, not the greatest idea in the world, but definitely not an error.

[czardas]

I consider it to be an error (of understanding if nothing else). It's like saying 2 + 2 = 4 + 0 + 0 + 0 + 0 + 0 + 0 + 0 + 0 + 0 + 0 + 0 + 0 + 0 + 0 + 0 + 0 + 0 + 0 + 0 + 0 + 0. Although the statement is true and the syntax is correct, it is still an error in my book due to redundancy.

I differentiate between the above and (for example) inappropriate (or the overuse of) global declarations. This I would classify as bad practice, although I have to admit that I find it hard to make a clear distinction between erroneous code and bad coding practice.

Edited December 4, 2013 by czardas

[mLipok]

 

It seems to me that you are just focussing on local variables declared within loops.

 

Exactly

... but only temporarily.

 

Wait a few minutes, maybe one hour.

I submit new version with Global Dim and Static and of course Local keyword.

I would consider this to be worse than bad practice because it produces repetition of redundant commands. I would classify this as a non-fatal error.

 

Ok I'll give you concrete examples of where you can find such cases:

 

in array.au3 (AuotIt 3.3.8.1 include file)

 

 

C:\Program Files (x86)\AutoIt3\Include\Array.au3
======================================================
While 1

Local $sClip = ""
Local $aiCurItems[1] = [0]

Local $aiCurItems[1] = [0]

For $x = 1 To 255

Local $sFind = _ArraySearch($avArray, Chr($x), 0, 0, 0, 1)

Local $sFind = _ArraySearch($avArray, Chr($x), 0, 0, 0, 1)

First example:

in Func _ArrayDisplay(....

focus on line #456 and #459

; Show dialog
    GUISetState(@SW_SHOW, $hGUI)

    While 1
        Switch GUIGetMsg()
            Case $_ARRAYCONSTANT_GUI_EVENT_CLOSE
                ExitLoop

            Case $hCopy
                Local $sClip = ""

                ; Get selected indices [ _GUICtrlListView_GetSelectedIndices($hListView, True) ]
                Local $aiCurItems[1] = [0]

Second example:

in Func _ArrayDisplay(....

focus on line #335

Func _ArrayDisplay(Const ByRef $avArray, $sTitle = "Array: ListView Display", $iItemLimit = -1, $iTranspose = 0, $sSeparator = "", $sReplace = "|", $sHeader = "")
    If Not IsArray($avArray) Then Return SetError(1, 0, 0)
    ; Dimension checking
    Local $iDimension = UBound($avArray, 0), $iUBound = UBound($avArray, 1) - 1, $iSubMax = UBound($avArray, 2) - 1
    If $iDimension > 2 Then Return SetError(2, 0, 0)

    ; Separator handling
    If $sSeparator = "" Then $sSeparator = Chr(124)

    ;  Check the separator to make sure it's not used literally in the array
    If _ArraySearch($avArray, $sSeparator, 0, 0, 0, 1) <> -1 Then
        For $x = 1 To 255
            If $x >= 32 And $x <= 127 Then ContinueLoop
            Local $sFind = _ArraySearch($avArray, Chr($x), 0, 0, 0, 1)
            If $sFind = -1 Then
                $sSeparator = Chr($x)
                ExitLoop
            EndIf
        Next
    EndIf
...
..
.

 

Tell me if you know about these ?

 

Can you focus on this and give any  proof that it is not only "BadCodingPractice" but also error ?

Edited December 4, 2013 by mlipok

[czardas]

Yeah, I think both an error and therefore (automatically) bad.

On the other hand, if the declaration is only encountered once, or infrequently (not true in this case), it may just about be acceptable to declare a variable within a loop in this way - perhaps sometimes. I would generally advise against doing so.

Edited December 4, 2013 by czardas

[mLipok]

I always considered Tidy a best coding practice analyzer.

 

You forgot to add, so I'll add my 2 cents

Tidy and Au3Check ...

 

Unfortunately, none of the above does not check the issues that I describe.

[mLipok]

and by the way

whether the new entry to the description in the opening post, is clear enough ?

[mLipok]

!!! NEW Version 2013/12/05 01:40 AM

• added checking for Global Dim and Static
• some other modyfication in REGEXP  pattern

 

see attached file in opening post

[JohnOne]

 

and by the way

whether the new entry to the description in the opening post, is clear enough ?

 

I have absolutely no idea what this script is for, from reading either the title, or the first post.

[mLipok]

Using this script you can analyze the old scripts and search for keywords like:

Local Global Dim Static

 

Search only the above. keywords used inside the Loop statements like:

 

For ... Next

While ... Wend

Do ... Until

For ... In ... Next

 

whether such a description is more clear?

Edited December 5, 2013 by mlipok

[JohnOne]

Yes.

So it parses a script and reports on unnecessary variable declarations within loops?

[mLipok]

exactly

but I would not say unnecessary, but rather incorrect.

 

EDIT:

Variable declaration is needed but it should be done outside the loop

 

EDIT2: 

So it parses a script and reports on unnecessary variable declarations within loops?

 

I used your word to describe in OP.

Thank you for the use of appropriate, descriptions.

Edited December 5, 2013 by mlipok

[jchd]

I beg to differ. Local variables within loops are not forcibly wrong practice. Blindly chasing such construct may result in much poorer code.

[mLipok]

And establishes an interesting discussion

Unfortunately, I personally do not have appropriate rebuttal arguments, the more the honorable speaker did not present their arguments.

[iamtheky]

what is the argument for declaring outside the loop, as opposed to assigning and iterating in the same location?

[JohnOne]

what is the argument for declaring outside the loop, as opposed to assigning and iterating in the same location?

I suppose it might ultimately depend on how the interpreter deals with it.

Although I'm not certain, I'd surmise that it could cause an extra operation to be taken with each iteration of a loop.

In a tight and frequent loop, that could cause unnecessary time and load issues.

[mLipok]

 As an argument for my solution to give an example of an old practice, which I found in this example:

in UDF - GuiTreeView.au3 - _GUICtrlTreeView_Sort

Func _GUICtrlTreeView_Sort($hWnd)
    If Not IsHWnd($hWnd) Then $hWnd = GUICtrlGetHandle($hWnd)

    Local $hItem, $a_tree
    For $i = 0 To _GUICtrlTreeView_GetCount($hWnd)
        If $i == 0 Then
            $hItem = _SendMessage($hWnd, $TVM_GETNEXTITEM, $TVGN_CHILD, $TVI_ROOT, 0, "wparam", "handle", "handle")
        Else
            $hItem = _SendMessage($hWnd, $TVM_GETNEXTITEM, $TVGN_NEXT, $hItem, 0, "wparam", "handle", "handle")
        EndIf
        If IsArray($a_tree) Then
            ReDim $a_tree[UBound($a_tree) + 1]
        Else
            Dim $a_tree[1]
        EndIf
        $a_tree[UBound($a_tree) - 1] = $hItem
    Next
    If IsArray($a_tree) Then
        Local $hChild, $i_Recursive = 1
        For $i = 0 To UBound($a_tree) - 1
            _SendMessage($hWnd, $TVM_SORTCHILDREN, $i_Recursive, $a_tree[$i], 0, "wparam", "handle") ; sort the items in root
            Do ; sort all the children
                $hChild = _SendMessage($hWnd, $TVM_GETNEXTITEM, $TVGN_CHILD, $hItem, 0, "wparam", "handle", "handle")
                If $hChild > 0 Then
                    _SendMessage($hWnd, $TVM_SORTCHILDREN, $i_Recursive, $hChild, 0, "wparam", "handle")
                EndIf
                $hItem = $hChild
            Until $hItem = 0x00000000
        Next
    EndIf
EndFunc ;==>_GUICtrlTreeView_Sort
 

For this example, I made a Feature Request TRACK TICKET #2554

 

---

 

btw.

 

on WIKI

http://www.autoitscript.com/wiki/Best_coding_practices

 

"Declaring variables in loops (For, While, Do etc..) can have an affect on efficiency:..."

[jchd]

Well I explicitely said "... is not forcibly (a bad thing)".

AutoIt doesn't enjoy the same fine-grain scoping rules as C (for instance). Yet I find it valuable to keep variable declarations as close to their use scope as if they were C variables. Of course, unless mother efficiency dictates otherwise, like in tight loops. This practice considerably help reading and maintaining the code.

Local $t = TimerInit()

Local $k1
For $i = 1 To 1000
    For $j = 1 To 1000
        $k1 = $i * $j
    Next
Next
ConsoleWrite(TimerDiff($t) & @LF)

$t = TimerInit()
For $i = 1 To 1000
    Local $k2
    For $j = 1 To 1000
        $k2 = $i * $j
    Next
Next
ConsoleWrite(TimerDiff($t) & @LF)

$t = TimerInit()
For $i = 1 To 1000
    For $j = 1 To 1000
        Local $k3
        $k3 = $i * $j
    Next
Next
ConsoleWrite(TimerDiff($t) & @LF)

On my old, slow PC, the difference between first and second arrangements is a bumping 60 ns (nanosecond) for each declaration. So unless I know a loop can/will turn a large number of times, I place declarations in the most natural place. Else I pretend it's premature optimization, just like if you limit your variables names to 2 characters because doing so speeds up the interpreter.

For arrays created in loops, re-declaration is pretty useful and (in my view) good practice as well.