Good coding practices in AutoIt

[czardas]

Some additional notes relating to reusing the loop variable. See by trancexx

MsgBox(0, "", "It's " & _GetTypeOfDay())

Func _GetTypeOfDay()
    ; Check if the current day is a normal working day - monday to friday.
    Local $iToday = @WDAY, $i ; Declaring the loop variable is not standard practice in AutoIt.
    For $i = 2 To 6
        If $i = $iToday Then ExitLoop
    Next

    ; The following variable '$sMessage' could also have been declared at the start of the function,
    ; but has been declared immediately prior to being used instead. [See Post #81 by guinness]
    Local $sMessage

    ; It is possible to use the loop variable outside the loop but it is not generally recommended.
    If $i < 7 Then ; Using the loop variable outside the loop is bad practice. [See post #87 by trancexx]
        $sMessage = "a normal working day" ; The loop was interrupted.
    Else
        $sMessage = "weekend" ; The loop ran from start to finish.
    EndIf

    Return $sMessage ; Single exit point or return value. [See post #63 by Mat]
EndFunc

The above code was intended to illustrate some of the other members comments, but had to be revised after advice from trancexx. Here's an alternative version which is better.

MsgBox(0, "", "It's " & _GetTypeOfDay())

Func _GetTypeOfDay()
    Local $sMessage = "a normal working day"
    ; Check if the current day is weekend - saturday or sunday.
    Switch @WDAY
        Case 1, 7
            $sMessage = "weekend"
    EndSwitch

    Return $sMessage ; Single exit point or return value. [See post #63 by Mat]
EndFunc

Edited January 25, 2013 by czardas

[Mat]

DRY and KISS, fail fast, SoC and single responsibility... Most of those are common sense and apply not just to programming. This on the other hand is a bit like the bible. You aren't expected to always follow it literally, stoning is generally frowned upon by modern society.

Single entry and exit (SESE). Usually overlooked with good reason but can be important, a GUI loop that looks like this is not good:

While 1
    Switch GUIGetMsg()
        Case $GUI_EVENT_CLOSE
            Exit
    EndSwitch
WEnd

This would be better:

While 1
    Switch GUIGetMsg()
        Case $GUI_EVENT_CLOSE
            ExitLoop
    EndSwitch
WEnd

Usually there are multiple places in the loop that should end the GUI (a cancel button, an action completing etc). If you were to add resources that need freeing, the second design means that code can go after the loop and be guaranteed to execute.

Same applies for functions.

Bad:

Func Foo($i)
    If $i < 12 Then Return "Foo"
    Return "Bar"
EndFunc

Better:

Func Foo($i)
    Local $ret = "Bar"
    If $i < 12 Then $ret = "Foo"

    Return $ret
EndFunc

You can now add code that will always be executed immediately before returning from the function (like freeing resources).

I'm not saying go for it and rewrite all your scripts like this. Trying to can result in larger and more unreadable code. But bear it in mind when you write your code.

[AZJIO]

Correctly (translate.google)

[guinness]

Make a habit of destroying resources whether they be native or UDF.

#include <GUIComboBox.au3>
#include <GUIConstants.au3>

Example()

Func Example()
    Local $hGUI = GUICreate('') ; Create a GUI with default parameters.
    Local $hCombo = _GUICtrlComboBox_Create($hGUI, '', 2, 2, 396, 296) ; Create a combobox using the combobox UDF.
    GUISetState(@SW_SHOW, $hGUI)

    While 1
        Switch GUIGetMsg()
            Case $GUI_EVENT_CLOSE
                ExitLoop
        EndSwitch
    WEnd

    _GUICtrlComboBox_Destroy($hCombo) ; Destroy the combobox.
    GUIDelete($hGUI) ; Destroy the GUI.
EndFunc   ;==>Example

[czardas]

Be consistant in your approach. Imagine you wrote some code like I did below.

_CheckBin(0x80000001)

Func _CheckBin($dHex)
    For $i = 0 To 31
        ConsoleWrite($i +1 & @TAB & (BitAND($dHex, 2^$i) = 2^$i) & @LF)
    Next
EndFunc

After running the code you spot a mistake. The oversight was that first the bit in a signed 32bit integer represents the minus sign. So you decide to write an exception as I did.

_CheckBin(0x80000001)

Func _CheckBin($dHex)
    For $i = 0 To 30 ; Method only works for up to 31 bits because 2^31 <> -2147483648
        ConsoleWrite($i +1 & @TAB & (BitAND($dHex, 2^$i) = 2^$i) & @LF)
    Next
    ConsoleWrite(32 & @TAB & ($dHex < 0) & @LF) ; This is inconsistant with the above method.
EndFunc

Now it works a treat But wait a minute! I thought I was being clever until I looked at my example a week later. What on Earth was I thinking? This code is hard to follow. Why? - Because it is unecessarily inconsistant. It would be easier to understand if I just used one method.

_CheckBin(0x80000001)

Func _CheckBin($dHex)
    For $i = 0 To 30 ; Method only works for up to 31 bits because 2^31 <> -2147483648
        ConsoleWrite($i +1 & @TAB & (BitAND($dHex, 2^$i) = 2^$i) & @LF)
    Next
    ConsoleWrite(32 & @TAB & (BitAND($dHex, 0x80000000) = 0x80000000) & @LF)
    ; Here we reach the limit imposed by 32bit integers on bitwise operations.
EndFunc

Probably still hard to follow, but at least it's consistant.

Edited January 26, 2013 by czardas

[Mat]

GUI design guidelines on MSDN.

Covers basic things like correct terminology. But it's things like that which speed up design time (particularly when working with other people). IT goes as far as to cover icon design (and how to do it in photoshop).

Even how to structure the text and title in a message box is covered. Since reading it I now get annoyed when an application presents me with an un-professional message box. It's a bit like this I guess.

Edit: I also like these guidelines because they are so definite. Like "Don't show a progress bar if the activity will be completed in less than 5 seconds." and they then say what you should use instead.

Edited January 23, 2013 by Mat

[guinness]

Just quickly browsing the articles and some of it is very informative. I mean they have 30+ years more experience than me so I'm sure I will learn something new about UI design.

Always place owned windows on top of their owner window. Never place owned windows under their owner windows, because most likely users won't see them

Kind of obvious.

[jaberwacky]

I used those guidelines when setting up the GUI for the SciTE Customization GUI script (mostly, kinda, ok I cherry picked).

Edited January 23, 2013 by jaberwocky6669

[BrewManNH]

Going back to what Mat said earlier about SESE (single entry single exit), I came up with a scenario using something simple like _ArrayAdd from the Array UDFs.

Here's the original function

Func _ArrayAdd(ByRef $avArray, $vValue)
     If Not IsArray($avArray) Then Return SetError(1, 0, -1) ; Exit point #1
     If UBound($avArray, 0) <> 1 Then Return SetError(2, 0, -1) ; Exit point #2
     Local $iUBound = UBound($avArray)
     ReDim $avArray[$iUBound + 1]
     $avArray[$iUBound] = $vValue
     Return $iUBound ; Exit point #3
EndFunc   ;==>_ArrayAdd

As you can see, it has 3 places where it exits back to the calling script, each one of them hard to find in the code if you don't know where to look or at least what to look for.

Here's a modified version of the same function using SESE.

Func _ArrayAdd(ByRef $avArray, $vValue)
     Local $iError = 0, $iUBound = -1
     If Not IsArray($avArray) Then $iError = 1
     If UBound($avArray, 0) <> 1  And Not $iError Then $iError = 2
     If Not $iError Then
          $iUBound = UBound($avArray)
          ReDim $avArray[$iUBound + 1]
          $avArray[$iUBound] = $vValue
     EndIf
     Return SetError($iError, 0, $iUBound)
EndFunc   ;==>_ArrayAdd

This has the exact same functionality as the first function, but uses a single point of exit. This is a minor change to the code, it's still able to be read by someone who knows what he's looking at, and adds 3 lines to the original script.

Edited January 23, 2013 by BrewManNH

[guinness]

Another example is _HexToString, suffice to say that has been fixed.

Edit: I don't know if SESE can be applied to _ArrayAdd, but I will investigate a little further on the rules. But this is why I created this thread to expand my knowledge of the good, the bad and the downright ugly.

Edited January 23, 2013 by guinness

[Mat]

I should have mentioned that errors are exceptions. I wouldn't expect the function to finish processing normally if an error occurs. Especially with parameter checking which I would expect to see at the top.

Like I said, don't follow it as a rule, it's really great to remember in long message loops or long functions that use resources, but the benefits in a 5 line function are far outweighed by the fact it's harder to read.

[czardas]

I'm not saying go for it and rewrite all your scripts like this. Trying to can result in larger and more unreadable code. But bear it in mind when you write your code.

So should I alter my code in post - or should I not bother about a single exit point? Am I confusing unrelated topics?

Edited January 24, 2013 by czardas

[guinness]

I found a good article whilst researching, but misplaced it. If I find it I will report back.

Edited February 13, 2013 by guinness

[Mat]

So should I alter my code in post - or should I not bother about a single exit point? Am I confusing unrelated topics?

Doesn't really matter, but it would be a lot neater with a single return if you used _Iif. Of course you probably wouldn't write that function like that anyway.

I was trying to make the point that if you Exit from the middle of a long message loop you are going to create problems.

[czardas]

Of course you probably wouldn't write that function like that anyway.

I was trying to make the point that if you Exit from the middle of a long message loop you are going to create problems.

Yeah it was a good post you made. It got me thinking. Thanks!

I wouldn't write the function the way I did niether. It just seemed a straight forward example to follow the logic. I might simplify it actually.

[guinness]

single return if you used _Iif.

Or ternary in the aplha/pre-beta! -_0 Edited January 24, 2013 by guinness

[Exit]

Or ternary in the beta! -_0

The current beta is 3.3.9.4 with no ternary

The alpha/pre-beta is 3.3.9.5 with ternary

[guinness]

The current beta is 3.3.9.4 with no ternary

The alpha/pre-beta is 3.3.9.5 with ternary

Changed. I'm currently using V3.3.9.5 so just presumed it was in the last version.

[guinness]

#include <Constants.au3>

Local $vValue ; Bad, this is initially blank.
Local $vVal = 0 ; Good

MsgBox($MB_SYSTEMMODAL, '', '$vValue: ' & $vValue & @CRLF & '$vVal: ' & $vVal)

; In C++ $vValue would display the previous value that was residing in the memory address $vValue points to. AutoIt
; is a little less forgiving in that it initialises the variable as being blank and doesn't throw any error, unlike some moder C++ compilers.

Edited January 25, 2013 by guinness

[guinness]

Declare variables when they are needed.

; Do I need to post an example?

Edited September 4, 2015 by guinness