Just reviewing what you've got here...
###Indentation
The code would read much better with proper indentation:
Function FillInternetForm()
Dim HWNDSrc As Long
Dim ie As Object
Set ie = CreateObject("InternetExplorer.Application")
HWNDSrc = ie.HWND
ie.Navigate "http://helppointinfo.farmersinsurance.com/OCR/Labor_Rates/laborrates.asp"
ie.Visible = True
While ie.Busy
DoEvents 'wait until IE is done loading page.
Wend
ie.Document.getElementById("DirectZip").Value = Sheets("NAT").Range("C2").Value
SetForegroundWindow HWNDSrc
Application.SendKeys "{TAB 11}", True
DoEvents
Application.SendKeys "{NUMLOCK}", True
End Function
Public Sub RunRates()
Call FillInternetForm
End Sub
###Call
Instruction
As answered in this StackOverflow question this StackOverflow question, the Call
instrucation is a relic from ancient versions of VB, it's not needed and, IMO, only adds clutter.
Public Sub RunRates()
FillInternetForm
End Sub
###Coupling
The FillInternetForm
function is needlessly coupled with the Excel object model - Sheets("NAT").Range("C2").Value
should be passed as a String
parameter to the function:
Function FillInternetForm(ByVal DirectZipValue As String)
'...
ie.Document.getElementById("DirectZip").Value = DirectZipValue
'...
End Function
Public Sub RunRates()
FillInternetForm Sheets("NAT").Range("C2").Value
End Sub
###Function?
VB functions are procedures with a return value. If it's not specified, then it's returning a Variant
- here FillInternetForm
is never assigned a return value, and whatever it would be returning wouldn't be used. In other words, you have a procedure (Sub
), not a function. The signature should be modified like this:
Public Sub FillInternetForm(ByVal DirectZipValue As String)
I like things explicit - if a member is going to be Private
, it needs a Private
access modifier; if it's going to be Public
, I don't like relying on VB's "defaults", mostly because I code in different languages where these defaults differ (C#). Having explicit access modifiers eliminate the possible confusion, but that might be only me.
Lastly, I don't understand why FillInternetForm
would have to press NUM LOCK, this looks misplaced, and has a side-effect that could be surprising to whoever is running that code.
Just reviewing what you've got here...
###Indentation
The code would read much better with proper indentation:
Function FillInternetForm()
Dim HWNDSrc As Long
Dim ie As Object
Set ie = CreateObject("InternetExplorer.Application")
HWNDSrc = ie.HWND
ie.Navigate "http://helppointinfo.farmersinsurance.com/OCR/Labor_Rates/laborrates.asp"
ie.Visible = True
While ie.Busy
DoEvents 'wait until IE is done loading page.
Wend
ie.Document.getElementById("DirectZip").Value = Sheets("NAT").Range("C2").Value
SetForegroundWindow HWNDSrc
Application.SendKeys "{TAB 11}", True
DoEvents
Application.SendKeys "{NUMLOCK}", True
End Function
Public Sub RunRates()
Call FillInternetForm
End Sub
###Call
Instruction
As answered in this StackOverflow question, the Call
instrucation is a relic from ancient versions of VB, it's not needed and, IMO, only adds clutter.
Public Sub RunRates()
FillInternetForm
End Sub
###Coupling
The FillInternetForm
function is needlessly coupled with the Excel object model - Sheets("NAT").Range("C2").Value
should be passed as a String
parameter to the function:
Function FillInternetForm(ByVal DirectZipValue As String)
'...
ie.Document.getElementById("DirectZip").Value = DirectZipValue
'...
End Function
Public Sub RunRates()
FillInternetForm Sheets("NAT").Range("C2").Value
End Sub
###Function?
VB functions are procedures with a return value. If it's not specified, then it's returning a Variant
- here FillInternetForm
is never assigned a return value, and whatever it would be returning wouldn't be used. In other words, you have a procedure (Sub
), not a function. The signature should be modified like this:
Public Sub FillInternetForm(ByVal DirectZipValue As String)
I like things explicit - if a member is going to be Private
, it needs a Private
access modifier; if it's going to be Public
, I don't like relying on VB's "defaults", mostly because I code in different languages where these defaults differ (C#). Having explicit access modifiers eliminate the possible confusion, but that might be only me.
Lastly, I don't understand why FillInternetForm
would have to press NUM LOCK, this looks misplaced, and has a side-effect that could be surprising to whoever is running that code.
Just reviewing what you've got here...
###Indentation
The code would read much better with proper indentation:
Function FillInternetForm()
Dim HWNDSrc As Long
Dim ie As Object
Set ie = CreateObject("InternetExplorer.Application")
HWNDSrc = ie.HWND
ie.Navigate "http://helppointinfo.farmersinsurance.com/OCR/Labor_Rates/laborrates.asp"
ie.Visible = True
While ie.Busy
DoEvents 'wait until IE is done loading page.
Wend
ie.Document.getElementById("DirectZip").Value = Sheets("NAT").Range("C2").Value
SetForegroundWindow HWNDSrc
Application.SendKeys "{TAB 11}", True
DoEvents
Application.SendKeys "{NUMLOCK}", True
End Function
Public Sub RunRates()
Call FillInternetForm
End Sub
###Call
Instruction
As answered in this StackOverflow question, the Call
instrucation is a relic from ancient versions of VB, it's not needed and, IMO, only adds clutter.
Public Sub RunRates()
FillInternetForm
End Sub
###Coupling
The FillInternetForm
function is needlessly coupled with the Excel object model - Sheets("NAT").Range("C2").Value
should be passed as a String
parameter to the function:
Function FillInternetForm(ByVal DirectZipValue As String)
'...
ie.Document.getElementById("DirectZip").Value = DirectZipValue
'...
End Function
Public Sub RunRates()
FillInternetForm Sheets("NAT").Range("C2").Value
End Sub
###Function?
VB functions are procedures with a return value. If it's not specified, then it's returning a Variant
- here FillInternetForm
is never assigned a return value, and whatever it would be returning wouldn't be used. In other words, you have a procedure (Sub
), not a function. The signature should be modified like this:
Public Sub FillInternetForm(ByVal DirectZipValue As String)
I like things explicit - if a member is going to be Private
, it needs a Private
access modifier; if it's going to be Public
, I don't like relying on VB's "defaults", mostly because I code in different languages where these defaults differ (C#). Having explicit access modifiers eliminate the possible confusion, but that might be only me.
Lastly, I don't understand why FillInternetForm
would have to press NUM LOCK, this looks misplaced, and has a side-effect that could be surprising to whoever is running that code.
Just reviewing what you've got here...
###Indentation
The code would read much better with proper indentation:
Function FillInternetForm()
Dim HWNDSrc As Long
Dim ie As Object
Set ie = CreateObject("InternetExplorer.Application")
HWNDSrc = ie.HWND
ie.Navigate "http://helppointinfo.farmersinsurance.com/OCR/Labor_Rates/laborrates.asp"
ie.Visible = True
While ie.Busy
DoEvents 'wait until IE is done loading page.
Wend
ie.Document.getElementById("DirectZip").Value = Sheets("NAT").Range("C2").Value
SetForegroundWindow HWNDSrc
Application.SendKeys "{TAB 11}", True
DoEvents
Application.SendKeys "{NUMLOCK}", True
End Function
Public Sub RunRates()
Call FillInternetForm
End Sub
###Call
Instruction
As answered in this StackOverflow question, the Call
instrucation is a relic from ancient versions of VB, it's not needed and, IMO, only adds clutter.
Public Sub RunRates()
FillInternetForm
End Sub
###Coupling
The FillInternetForm
function is needlessly coupled with the Excel object model - Sheets("NAT").Range("C2").Value
should be passed as a String
parameter to the function:
Function FillInternetForm(ByVal DirectZipValue As String)
'...
ie.Document.getElementById("DirectZip").Value = DirectZipValue
'...
End Function
Public Sub RunRates()
FillInternetForm Sheets("NAT").Range("C2").Value
End Sub
###Function?
VB functions are procedures with a return value. If it's not specified, then it's returning a Variant
- here FillInternetForm
is never assigned a return value, and whatever it would be returning wouldn't be used. In other words, you have a procedure (Sub
), not a function. The signature should be modified like this:
Public Sub FillInternetForm(ByVal DirectZipValue As String)
I like things explicit - if a member is going to be Private
, it needs a Private
access modifier; if it's going to be Public
, I don't like relying on VB's "defaults", mostly because I code in different languages where these defaults differ (C#). Having explicit access modifiers eliminate the possible confusion, but that might be only me.
Lastly, I don't understand why FillInternetForm
would have to press NUM LOCK, this looks misplaced, and has a side-effect that could be surprising to whoever is running that code.