Improving My Coding Style

[fu2m8]

Hey Guys,

Just wanna say ive been a long time lurker on the forums and it seems like a great friendly place to learn a lot .

Soon at work i'll probaly have to be scripting a lot of things in AutoIT, and if any1 has the time ill post up a couple of scripts I made a year or so ago for any1 to point out mistakes/formatting errors/useless/excess/ code that i've undoubtedly let slip in (i don't come from a programming background), so any and all critiscisms and pointers are most appreciated and welcome . These scripts were working in the environment i set them up in but could probaly be made more portable with more efficient code.

Anywho the first was a program that copied a certain background (from the network) to the user's computer as the desktop wallpaper (used it in a school environment where the schools grades were assigned animals, eg. Kindy would be Koala's so they would have a picture of a Koala, Yr 1 would be Emu's and have a picture of an Emu etc . It just helped the kids recongise that they had logged into the right user account -- this school didnt have roaming profiles ... ) .

expandcollapse popup

;Checks to see if the Operating System is Windows XP
OS_check()

Func OS_check()
    $operating_system = @OSVersion;@OSVersion returns what Windows Operating System the user is using
    If $operating_system = "WIN_XP" Then;Just checks the $operating_system variable to make sure it is Windows XP
        bg_id();If so it proceeds to bg_id() function
    Else
        MsgBox(64, "Error #OS01: Operating System Unsupported", "Currently only Windows XP is supported by this program. Cheers, The IT Guy")
        Exit
    EndIf
EndFunc

;Checks to see if bg_id.ini file exists in C:\Documents and Settings\username\Local Settings\Application Data\Microsoft (this is where Windows places converted images that are used as backgrounds, it also converts them to BMP (bitmap) files
Func bg_id()
    $program_bgid = IniRead(@ScriptDir & "\bgsettings.ini", "bgc", "bgid", "0");@ScriptDir returns the path that the script is running from without a trailing backslash, it then points to the bgsettings.ini file which must be placed in the same folder for the script to run
    If $program_bgid = "0" Then
        MsgBox(64, "Error #INIr01: Unable to read the 'bgsettings.ini' file.", "The program was unable to read the 'bgsettings.ini' file.  Maybe it has been deleted or removed. The program will now exit.")
        Exit
    EndIf
    If FileExists(@UserProfileDir & "\Local Settings\Application Data\Microsoft\bgid.ini") Then;This checks to see if the bgid.ini file exists, if not it proceeds copy_bg() function, if it does exist it checks to see if the bgid values are the same, if so it exits the program, if not it proceeds to copy_bg(0 function
        $local_bgid = IniRead(@UserProfileDir & "\Local Settings\Application Data\Microsoft\bgid.ini", "bgc", "bgid", "0")
        If $local_bgid = $program_bgid Then
            Exit
        Else
            copy_bg();Proceeds to copy_bg() function if the values are different
        EndIf
    Else
        copy_bg();Proceeds to copy_bg() function if no bgid.ini file exists (it will be created in that function)
    EndIf
EndFunc

;Copies a file named 'Wallpaper1.bmp' to the users profile directory into C:\Documents and Settings\username\Local Settings\Application Data\Microsoft, the wallpaper must be named Wallpaper1.bmp and must reside in the same file that the application is run from 
Func copy_bg()
    $bg_copy = FileCopy(@ScriptDir & "\Wallpaper1.bmp", @UserProfileDir & "\Local Settings\Application Data\Microsoft", 1)
    If $bg_copy = 0 Then
        MsgBox(64, "Error #FC01: Unable to copy the Background Picture", "Sorry but the program was unable to successfully copy the background picture to the User's Profile directory. The program will now exit.")
        Exit
    EndIf
    $changed_bgid = IniRead(@ScriptDir & "\bgsettings.ini", "bgc", "bgid", "0");Checks the new bgid value in the bgsettings.ini file from the program directory, it will use this same value on the local machine in the bgid.ini file. The next time the program is run if these to values are the same the program will exit and not bother copying the background 
    If $changed_bgid = "0" Then
        MsgBox(64, "Error #INIr04: Unable to read the 'bgsettings.ini' file.", "The program was unable to read the 'bgsettings.ini' file.  Maybe it has been deleted or removed. The program will now exit.")
        Exit
    EndIf
    $bgid_iniwrite = IniWrite(@UserProfileDir & "\Local Settings\Application Data\Microsoft\bgid.ini", "bgc", "bgid", $changed_bgid);Writes the value of $changed_bgid to the bgid.ini file used for comparison purposes for future runnings of the program
    If $bgid_iniwrite = 0 Then
        MsgBox(64, "Error #INIw01: Unable to write to the 'bgid.ini' file.", "The program was unable to write to the 'bgid.ini' file in the user's profile directory. The program will now exit.")
        Exit
    EndIf
    registry_change();Proceeds to the registry_change() function if the copy is successful
EndFunc


;Changes the Regsitry for the current user to point the background to the file that was just copied
Func registry_change()
    $bg_registry = RegRead("HKEY_CURRENT_USER\Control Panel\Desktop", "Wallpaper");Reads the current value of HKEY_CURRENT_USER\Control Panel\Desktop\Wallpaper 
    If $bg_registry = @UserProfileDir & "\Local Settings\Application Data\Microsoft\Wallpaper1.bmp" Then;If it already points to the users profile directory (@UserProfileDir\Local Settings\Application Data\Microsoft\Wallpaper1.bmp then the program exits here) it assumes the the originalwallpaper setting is also the same
        Exit
    Else
        $bg_regwrite1 = RegWrite("HKEY_CURRENT_USER\Control Panel\Desktop", "Wallpaper", "REG_SZ", @UserProfileDir & "\Local Settings\Application Data\Microsoft\Wallpaper1.bmp");If it doesn't already point here it updates to registry entries, to point to the copied wallpaper
        If $bg_regwrite1 = 0 Then
            MsgBox(64, "Error #REG01: Unable to write value to registry", "Sorry but the program was unable to write the value to the registry. The program will now exit.")
            Exit
        EndIf
        $bg_regwrite2 = RegWrite("HKEY_CURRENT_USER\Control Panel\Desktop", "OriginalWallpaper", "REG_SZ", @UserProfileDir & "\Local Settings\Application Data\Microsoft\Wallpaper1.bmp")
        If $bg_regwrite2 = 0 Then
            MsgBox(64, "Error #REG02: Unable to write value to registry", "Sorry but the program was unable to write the value to the registry. The program will now exit.")
            Exit
        EndIf       
    EndIf
EndFunc

Exit

and the second was pretty similar (cant find the code for any others atm ) in that it copied a background picture that users would see before they log onto a machine (the one where you have to press ctrl+alt+del.

expandcollapse popup

; ----------------------------------------------------------------------------
;
; AutoIt Version: 3.1.0
; Author: The IT Guy
;
; Script Function:
;   Copies a background picture to the c:\unzipped folder that is used to dispaly a background picture before the user logs on
;
; ----------------------------------------------------------------------------

; Script Start - Add your code below here

OS_check()

Func OS_check()
    $operating_system = @OSVersion;@OSVersion returns what Windows Operating System the user is using
    If $operating_system = "WIN_XP" Then;Just checks the $operating_system variable to make sure it is Windows XP
        admin();If so it proceeds to admin() function
    Else
        Exit
    EndIf
EndFunc

Func admin()
  ;If they are not an Admin on the local machine a MsgBox is displayed saying so
   If Not IsAdmin() Then
      Exit
  EndIf
    backgroundpicexists()
EndFunc
    
Func backgroundpicexists()
    $bglogonpicexist = FileExists("c:\backgroundpic\hs_logon_background_pic.bmp")
    If $bglogonpicexist = 1 Then
        registry_check()
    Else
        bg_logon_copy()
    EndIf
EndFunc

Func bg_logon_copy()
    DirCreate("c:\backgroundpic")
    $bg_copy = FileCopy("I:\Applications\bgchange\logonbackgroundpic\hs_logon_background_pic.bmp", "c:\backgroundpic\", 1)
    If $bg_copy = 1 Then
        registry_check()
    Else
        MsgBox(48, "Error #FC01: The File Could Not Be Copied", "The logon background changer program was unable to successfully copy the picture. Perhaps the directory doesn't exist or is read only. Check to make sure the 'backgroundpic' directory exists")
        Exit
    EndIf
EndFunc

Func registry_check()
    $registry_read = RegRead("HKEY_USERS\.DEFAULT\Control Panel\Desktop", "Wallpaper")
    If $registry_read = "c:\backgroundpic\hs_logon_background_pic.bmp" Then
        Exit
    Else
        $registry_write = RegWrite("HKEY_USERS\.DEFAULT\Control Panel\Desktop", "Wallpaper", "REG_SZ", "c:\backgroundpic\hs_logon_background_pic.bmp")
        If $registry_write = 1 Then
            Exit
        Else
            MsgBox(48, "Error #REG01: Error Writing to the Registry", "The logon background changer program was unable to write to the registry successfully. The program will now exit.")
            Exit
        EndIf
    EndIf
EndFunc

Exit

Anyway i would love to learn how to improve my code!

Thanks Guys (and Jon/Devs for such a great easy to pick up language )

[AutoItKing]

Try using Tidy. Comes with SciTe. I use it all the time. Makes my code look.....better.

[neogia]

Oh, and a very friendly name you have there, too.

[SmOke_N]

Try using Tidy. Comes with SciTe. I use it all the time. Makes my code look.....better.

Well for not using Tidy, I only saw one indentation issue on line 27 for "exit", so that's not really a factor IMHO, that's pretty damn good if you ask me.

I didn't see any issues with the first script other than not declaring your variables in a local scope for those functions, but that's not really an issue either. (don't know if you were using them in a global or not).

On the 2nd script "the similar one", I noticed you assumed the end-user was using "C:\" as their drive, that definately can/will be an issue. Other than that, I thought you did well.

One thing I'll say, is if you are going to have 3 or 4 line functions, and only be calling them from one function, you might just consider making it one big function.

I didn't test it because of the registry writing, but I didn't see any "Coding" issues.

Take Care... and quit lurking

[JSThePatriot]

As stated, I rarely use Tidy myself because I keep my code structured the way I prefer it. I use Capitolization, and Tabs and all of that to keep my code as readable as possible.

I agree with SmOke_N on the functions.

Another thing you might consider are some ;comments. They generally help others read your code later should there be a need. (I am not very diciplined in this area myself).

Also your code is structured well, but maybe consider structuring your file.

Here is a template I generally use...

expandcollapse popup

;>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
;---*** Preprocessor ***---
;<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<
#include <GUIConstants.au3>

;>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
;---*** File Installations ***---
;<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<

;>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
;---*** Registry Information ***---
;<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<

;>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
;---*** Declare Variables ***---
;<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<

Global $wHeight, $wWidth

;>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
;---*** Define Variables ***---
;<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<

$wHeight = 
$wWidth  = 

;>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
;---*** GUI Creation ***---
;<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<


;>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
;---*** Main Program Loop ***---
;<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<

While 1
    $msg = GUIGetMsg()
    Select
        Case $msg = $GUI_EVENT_CLOSE
            ExitLoop
    EndSelect
WEnd

GUIDelete($m_HWnd)

Exit

;>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
;---*** Define Functions ***---
;<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<

That's for GUI's, and if I am just making a script then I just remove the GUI stuff, but atleast now all my functions are at the end of the script, all my Preprocessor's are at the beginning... stuff like that.

Just my opinions,

JS

[fu2m8]

thanks for the tips will keep them in mind i think the reason i originally had everything in it's own little function was because i found it easier in my head to put all the bits of logic in their own block (function) and then when i got the script working i probaly just left it (don't fix it if it isn't broken mentality ) will definately check out tidy sounds really good...

On a side note, work also asked me to do some stuff in VBscript and is it just me or things that you can code in AutoIt in one line, take at least 5 lines in VB?!?

[jftuga]

Instead of

If $bg_regwrite1 = 0 Then

you should consider

If 0 = $bg_regwrite1 Then

If you ever start programming in languages such as C or PHP, where both

if b = 0

and

if b == 0

are legal, you will run into less logic errors. Most programmers want b==0 and not b=0 (usually a mistake), however 0==b is legal and logically correct while 0=b will give you an lvalue type compiler error.

Hope this helps!

-John