Simplifying Block of Code using Variables Topic is solved

[Thoughtfu1Tux]

I have written a billing script for my work that grabs data off an excel sheet, display a GUI checkbox list that lets me select specific names that I would like to bill for and then runs a Selenium script that will then bill for each person that I have selected from the GUI checkbox. The part of my code that checks which person I selected from the GUI checklist repeats itself 20 times as I have not been able to figure out how to simplify it because of the multiple labels that are used together.

Here is what my code looks like. I have shortened it to the first 4 people to make it easier to read. If there is anything else I can provide to make it easier to understand, please let me know.

I would appreciate the help, thank you!

Code: Select all

;Excel Data Initial data grab starts here (Only grabs the client names that will be displayed in the GUI checklist)
WinActivate, ProgramBillingWorkbook.xlsm - Excel
Xl := ComObjActive("Excel.Application") ;creates a handle to your currently active excel sheet
	P2 := Xl.Range("E2").Value
	P3 := Xl.Range("E3").Value
	P4 := Xl.Range("E4").Value
	P5 := Xl.Range("E5").Value

Gui, Add, Checkbox, vClient2 gSubmitGUI, %P2%		;P2 - P5 is a name grabbed from an Excel list using COM. There is no P1 as Excel uses row 1 for labels.
Gui, Add, Checkbox, vClient3 gSubmitGUI, %P3%		
Gui, Add, Checkbox, vClient4 gSubmitGUI, %P4%
Gui, Add, Checkbox, vClient5 gSubmitGUI, %P5%

Startbilling:	;Runs through the client# variables and checks if they were checked and then bills for them if they were. Otherwise moves on to the next one. NEED TO REDO because code is inneficient. 

	if(Client2 = 1)	;Checks to see if first person on list was checked or not (1 = Checked, 0 = Unchecked)
	{	
		Client2 = 0	;Sets checkmark to 0, (unchecked) and then proceeds to billing. 
		Num = 2		;Sets Variable equal to Client # that is Checked for GrabData sub.
		Gosub, GrabData
	}
	if(Client3 = 1)	
	{	
		Client3 = 0
		Num = 3
		Gosub, GrabData
	}
	if(Client4 = 1)	
	{	
		Client4 = 0
		Num = 4
		Gosub, GrabData
	}
		if(Client5 = 1)	
	{	
		Client2 = 0
		Num = 5
		Gosub, GrabData		
	}
	;Code repeats in the same matter until Client20
	else 	;If Client2 - Client20 are all equal to 0, then do this. 
	{
		msgbox,  that's all of them. Click OK to exit script. 
		ExitApp
	}

	GrabData:	;Grabs data from Excel sheet and saves them as variables based on the number stored in variable NUM ;{
		WinActivate, ProgramBillingWorkbook.xlsm - Excel
		Xl := ComObjActive("Excel.Application") ;creates a handle to your currently active excel sheet
		CliendID := Xl.Range("D" Num).Value		;Grabs ClientID
		AuthNumber := Xl.Range("A" Num).Value
		ServCode := Xl.Range("G" Num).Value
		;etc. etc. Code grabs a bunch of lines, all in the same fashion as the above examples. 
		
SeleniumAutomation: ;Inputs data into Chromium and then clicks the submit button. Removed the code as it's not really necessary for the example. 
	gosub, StartBilling	;Loops back to StartBilling sub after Selenium automation is complete.

[lexikos]

I would recommend reading about Functions, and using those instead of gosub. Functions are designed to accept parameters. For example:

Code: Select all

GrabData(Num)
{
    WinActivate, ProgramBillingWorkbook.xlsm - Excel
    Xl := ComObjActive("Excel.Application") ;creates a handle to your currently active excel sheet
    CliendID := Xl.Range("D" Num).Value		;Grabs ClientID
    AuthNumber := Xl.Range("A" Num).Value
    ServCode := Xl.Range("G" Num).Value
    ;etc. etc. Code grabs a bunch of lines, all in the same fashion as the above examples. 
}

To call (execute) the function for client 2, you would use GrabData(2). After the function is executed (down to the closing brace or return if there is one), the script will resume at the next line after GrabData(2).
For example:

Code: Select all

    if(Client2 = 1)	;Checks to see if first person on list was checked or not (1 = Checked, 0 = Unchecked)
        GrabData(2)
    if(Client3 = 1)	
        GrabData(3)
    ;...
    msgbox,  that's all of them. Click OK to exit script. 
    ExitApp

To reduce the repetition:

Code: Select all

Loop 4 {
    n := A_Index+1
    if Client%n%
        GrabData(n)
}

When checking something that's either 1 (checked/on/yes/true) or 0 (unchecked/off/no/false), you can use if Client2 in place of if(Client2 = 1).

You can do the same when constructing the GUI:

Code: Select all

Loop 4 {
    n := A_Index+1
    Gui, Add, Checkbox, vClient%n% gSubmitGUI, % xl.Range("E" n).Value
}

It could be simplified slightly by numbering the variables 1..4 and just adding 1 when retrieving the cell value: Range("E" A_Index+1).

Does the ;etc. etc. include a return? If not (i.e. if execution continues from GrabData into SeleniumAutomation, as it would in the code you posted), you are using gosub incorrectly. Gosub executes a subroutine and returns to the line after Gosub when a return is encountered. You should not explicitly "gosub" (or "goto") back to the beginning; instead, you should return from the subroutine. Each time you gosub, it remembers its place so that return can bring it back there. If you repeatedly gosub without ever returning, the program's "call stack" will run out of space and the program will crash. (This usually takes several hundred gosubs, so won't happen in this particular script.) Worse, if you both "gosub" back to the beginning and also eventually return, you may end up executing parts of the code more times than intended. Functions are the same, except that return is implied by the closing brace.

[Thoughtfu1Tux]

[quote="lexikos"][/quote]

Wow!
Thank you for the help!! I just incorporated all the code that your recommended into my script and ended up removing 350 lines of code.
The amazing thing about your solution is that if I ever want to show/bill for more than 20 people I could just Increase the loop amount, or just make the loop # into a variable and set that at the beginning of the script. WOW!

I know a little bit about functions, I actually have an #Include LoginTowebsite.ahk at the top of this script, and then the Login function gets called at the beginning of the script, but I didn't even think about using a function in the way that you recommended above though.

Good catch on the gosubs, I did not know about the returns. In the last 8 months that I have been learning about AutoHotKey I have been reading just the parts of the documentation that I could understand and skipping the extra info as a lot of it didn't make sense to me with my very limited experience with programming and computer science, it's a habit i'm trying to get myself out of now.

Thank you again for the huge help!!

[DRocks]

This is a gem for any similar type of needs. Thanks for asking the question. And thanks Lexikos for the long detailed reply. Its super helpful to analyze it.

PS: i also do like you for learning lol. The more you learn the more mystical talks you begin to understand its fun and gratifying hehe
AHK is awesome