Skip to main content
Code Review

Return to Answer

replaced http://stackoverflow.com/ with https://stackoverflow.com/
Source Link

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.

Source Link
Mathieu Guindon
  • 75.5k
  • 18
  • 194
  • 467

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.

lang-vb

AltStyle によって変換されたページ (->オリジナル) /