Is this bad pratice

[IanN1990]

This is just an off question as i am not sure. Here is a random function i wrote.

#NoTrayIcon
ConsoleWrite(Timer())

Func Timer()
$Temp = TimerInit()
While Not WinExists('Notepad')
If TimerDiff($Temp) > 5000 Then Return 1
sleep(250)
WEnd
Return 0
EndFunc

Is it bad returning a function, while being inside a Loop or should i exit the loop first before returning?

#NoTrayIcon
ConsoleWrite(Timer())

Func Timer()
$Temp = TimerInit()
While Not WinExists('Notepad')
If TimerDiff($Temp) > 5000 Then ExitLoop
sleep(250)
WEnd
If TimerDiff($Temp) > 5000 Then Return 1
If TimerDiff($Temp) < 5000 Then Return 0
EndFunc

Though sense functions return 0 by default, it could be this.

#NoTrayIcon
ConsoleWrite(Timer())

Func Timer()
$Temp = TimerInit()
While Not WinExists('Notepad')
If TimerDiff($Temp) > 5000 Then ExitLoop
sleep(250)
WEnd
If TimerDiff($Temp) > 5000 Then 1
EndFunc

Edited February 12, 2013 by IanN1990

[guinness]

This is correct in my humble opinion.

#NoTrayIcon

ConsoleWrite(Timer() & @CRLF) ; Could use WinWait of course!

Func Timer()
    Local Const $hTimer = TimerInit() ; Create a timer variable.
    Local $fNotepadExists = False ; Create a boolean variable.
    While Not $fNotepadExists ; While not Notepad window exists.
        $fNotepadExists = WinExists('[CLASS:Notepad]') = 1
        If TimerDiff($hTimer) > 5000 Then
            ExitLoop
        EndIf
        Sleep(10)
    WEnd
    Return $fNotepadExists
EndFunc   ;==>Timer

[IanN1990]

I was aware of WinWait and i do normally use built in functions when ever possible but there is no timer for what i am writting. So i made an example to demostrate if my style was wrong or harmful.

Looking at your example 3 questions come to mind.

Why declare the TimerInIt() as a local const ? Its only used for the function and is thrown away once the function ends. Much like

$i = 0 to 10

Next

Where you wouldn't locally declare the $i? (The only reason i could think of is incase the function is called again before it ends, though this could cause a recursion which i try and avoid like the plague)

Is there anything wrong with using a check statement, for a While Loop. The way i saw it, if notepad doesn't exist it would equal = 1. So the while loop carrys on until the timer is reached or it does exist "makin it 0" and the while loop ends.

Thirdly, does this mean i should have a exitloop ? or is returning while inside a loop considered bad or dangerous?

Edited February 12, 2013 by IanN1990

[guinness]

Most of the questions can be answered by navigating to the link in my signature called "Coding" but I will answer them here for you.

Why declare the TimerInIt() as a local ? Its only used for the function and is thrown away once the function ends. Much like

$i = 0 to 10

Next

You wouldn't locally declare the $i?

Because $i is in 'Loop' scope so it's automatically Local. Explicitly using Local or Global ensures that you have control over variable declaration. If you had a variable called $hTimer in Global scope and we didn't use the Local keyword, then this Global variable would be overwritten causing massive issues if that variable contained important data. By default if you don't use Local in a function and the variable doesn't exist in Global, then it's automatically Local. Oh and don't use Dim, first post of that link explains why.

Look at "single entry point, single exit point" normally it's better to return from a function quickly if there is an error, but in your example returning whilst in the loop is considered bad practice and poses as many unnecessary exit points. Of course as I said sometimes you need to, it's just a question of using your common sense of what is easier to understand and doesn't conform to SESE vs conforms with SESE but is difficult to read.

Here look at this as spot the difference.

#include <GUIConstantsEx.au3>
#include <WindowsConstants.au3>

Example()

Func Example()
    ; Create a GUI with various controls.
    Local $hGUI = GUICreate("Example - Bad")
    Local $iOK = GUICtrlCreateButton("OK", 310, 370, 85, 25)

    ; Display the GUI.
    GUISetState(@SW_SHOW, $hGUI)

    While 1
        Switch GUIGetMsg()
            Case $GUI_EVENT_CLOSE, $iOK
                Exit ; Bad as the GUI controls are never deleted. Even though AutoIt takes care of this, we shouldn't be too reliable on AutoIt as well as lazy.

        EndSwitch
    WEnd

    ; Delete the previous GUI and all controls.
    GUIDelete($hGUI)
EndFunc   ;==>Example

#include <GUIConstantsEx.au3>
#include <WindowsConstants.au3>

Example()

Func Example()
    ; Create a GUI with various controls.
    Local $hGUI = GUICreate("Example - Good")
    Local $iOK = GUICtrlCreateButton("OK", 310, 370, 85, 25)

    ; Display the GUI.
    GUISetState(@SW_SHOW, $hGUI)

    While 1
        Switch GUIGetMsg()
            Case $GUI_EVENT_CLOSE, $iOK
                ExitLoop ; Gpod as the GUI controls can be tidied up.

        EndSwitch
    WEnd

    ; Delete the previous GUI and all controls.
    GUIDelete($hGUI)
EndFunc   ;==>Example

You could use: but GUIDelete at least takes care of this for the controls.

; Delete the previous GUI and all controls.
    GUICtrlDelete($iOK)
    GUIDelete($hGUI)

Edited February 12, 2013 by guinness

[IanN1990]

I was also aware of your topic for good coding pratices, which is what sparked this topic in a way as i wasn't sure how to take it.

Because $i is in 'Loop' scope so it's automatically Local. Explicitly using Local or Global ensures that you have control over variable declaration. If you had a variable called $hTimer in Global scope and we didn't use the Local keyword, then this Global variable would be overwritten causing massive issues if that variable contained important data. By default if you don't use Local in a function and the variable doesn't exist in Global, then it's automatically Local. Oh and don't use Dim, first post of that link explains why.

I found that part really useful, i had no idea that could happen and though it doesn't affect any of my code it is sure something i will be careful about in the furture.

And i would agree Autoit is far to forgiving when it comes to cleaning up resources. I do try to make it a pratice as well as cleaning up as i go.

[guinness]

I was also aware of your topic for good coding pratices, which is what sparked this topic in a way as i wasn't sure how to take it.

Great, I wish more people would start topics of this nature.

[Mat]

You should always assume a function can be called from anywhere, and it should not affect the program.

In an ideal world, you should be able to run this code in any of the examples:

expandcollapse popup

#include <GUIConstantsEx.au3>
#include <WindowsConstants.au3>


Local $gui = GUICreate("Run the example?", 400, 300)
Local $btn = GUICtrlCreateButton("Run it", 20, 20, 80, 30)

GUISetState()

Local $msg
While 1
    $msg = GUIGetMsg()
    Switch $msg
        Case $GUI_EVENT_CLOSE
            ExitLoop
        Case $btn
            Example()
    EndSwitch
WEnd


Func Example()
    ; Create a GUI with various controls.
    Local $hGUI = GUICreate("Example - Good")
    Local $iOK = GUICtrlCreateButton("OK", 310, 370, 85, 25)

    ; Display the GUI.
    GUISetState(@SW_SHOW, $hGUI)

    While 1
        Switch GUIGetMsg()
            Case $GUI_EVENT_CLOSE, $iOK
                ExitLoop ; Gpod as the GUI controls can be tidied up.

        EndSwitch
    WEnd

    ; Delete the previous GUI and all controls.
    GUIDelete($hGUI)
EndFunc   ;==>Example

It's not quite perfect, as there are a few extra considerations when designing multi-window interfaces that really would make the examples too complicated to be of any use (open the example window, then click the close button on the first window for example).