我有一个代码,从一个名为X费率的网站获取费率,并输出超过选定国家月平均水平的数据。
代码运行得相当快,但我仍然认为我可以对代码做一些改进,但不知道该寻找什么。我已经做了一些显而易见的事情,比如使选项显式化和禁用屏幕更新。有人能指出我的缺点吗?
此外,您将看到代码使用if's而不是select case。这会不会是对atm的一个改进呢?
为长代码道歉,但如果你帮我,我将永远感激!
Option Explicit
Sub fetchCurrencyPast()
'Define variables
Dim a As Integer
Dim b As Integer
Dim c As Integer
Dim d As Integer
Dim i As Integer
Dim boolCtrl As Boolean
Dim period As Variant
Dim sCrcy As Variant
Dim MsgErr As String
'Error handler
On Error GoTo ErrHandler
'Apply format text on cells, and centre it.
'Change format to text
Columns("A:F").Select
With Selection
.HorizontalAlignment = xlCenter
.NumberFormat = "@"
End With
'Clear selection
Cells(1, 1).Select
'Add header
Range("A1", "F1").Style = "Input"
Range("A1", "F1").Font.Bold = True
Cells(1, 1).Value = "Year"
Cells(1, 2).Value = "OffSetCurr"
Cells(1, 3).Value = "Month"
Cells(1, 4).Value = "toEuro"
Cells(1, 5).Value = "toDollars"
Cells(1, 6).Value = "toPounds"
'Define flag for error
boolCtrl = False
'Define date and format as date
period = Application.InputBox("What's the year you want to collect back data?", "Period", , , , , 2)
On Error GoTo ErrHandler
'Error control on period
If Len(period) <> 4 Then
boolCtrl = True
GoTo ErrHandler
Exit Sub
End If
'Make the code faster
Application.ScreenUpdating = False
'Start fetching values from each country
For i = 1 To 9
'Define start row
a = 2
c = 2
'Define start col
b = 4
d = 3
If i = 1 Then
'ARS
Cells(a, 2).Value = "ARS"
Cells(a, 1).Value = period
For Each sCrcy In Array("EUR", "USD", "GBP")
Call GetRateYear("ARS", sCrcy, period, a, b)
a = 2
b = b + 1
c = a
Call GetSingleMonth("ARS", sCrcy, period, c, d)
Next
End If
If i = 2 Then
a = 14
b = 4
'AUD
Cells(a, 2).Value = "AUD"
Cells(a, 1).Value = period
For Each sCrcy In Array("EUR", "USD", "GBP")
Call GetRateYear("AUD", sCrcy, period, a, b)
a = 14
b = b + 1
c = a
Call GetSingleMonth("AUD", sCrcy, period, c, d)
Next
End If
'Other ifs afterwards for each country
错误处理程序:
ErrHandler:
If Err.Number <> 0 Then
MsgErr = "Error #" & Str(Err.Number) & " was generated by " & Err.Source & "." & Chr(13) & "Error description: " & Err.Description
MsgBox MsgErr, , "Error", Err.HelpFile, Err.HelpContext
Exit Sub
End If
If boolCtrl = True Then
MsgBox "Wrong date. Please retry!", vbCritical + vbOKOnly, "Error found!"
End If
End Sub
函数GetRateYear:
Function GetRateYear(sFromCrcy, sToCrcy, sYear, a, b)
'This function sends a XML HTTP request, as is much more faster than waiting for browser to DoEvents
'Usage -> Goes to X-rates website and retrieves the code from conversion
Dim sUrl, sContent, intMatches
Dim mtchCnt As Integer
Dim subMtchCnt As Integer
sUrl = "http://www.x-rates.com/average/?from=" & sFromCrcy & "&to=" & sToCrcy & "&amount=1&year=" & sYear
'XML Object that queries the website and retrieves HTML as text
With CreateObject("MSXML2.XMLHttp")
.Open "GET", sUrl, False
.send
sContent = .responseText
End With
'This retrieves values of currency (until end with)
With CreateObject("VBScript.RegExp")
.Global = True
.MultiLine = True
.IgnoreCase = False
.Pattern = "<span class=""avgRate"">(.*?)</span>"
'To do the count, you must always execute the regex first
Set intMatches = .Execute(sContent)
If intMatches.Count <> 0 Then
With intMatches
For mtchCnt = 0 To .Count - 1
For subMtchCnt = 0 To .Item(subMtchCnt).SubMatches.Count - 1
GetRateYear = .Item(mtchCnt).SubMatches(0)
Cells(a, b).Value = GetRateYear
Cells(a, 1).Value = sYear
Cells(a, 2).Value = sFromCrcy
a = a + 1
Next
Next
End With
End If
End With
End Function
函数GetSingleMonth
Function GetSingleMonth(sFromCrcy, sToCrcy, sYear, c, d)
'This function sends a XML HTTP request, as is much more faster than waiting for browser to DoEvents
'Usage -> Goes to X-rates website and retrieves the code from conversion
Dim sUrl, sContent, intMatches
Dim mtchCnt2 As Integer
Dim subMtchCnt2 As Integer
sUrl = "http://www.x-rates.com/average/?from=" & sFromCrcy & "&to=" & sToCrcy & "&amount=1&year=" & sYear
'XML Object that queries the website and retrieves HTML as text
With CreateObject("MSXML2.XMLHttp")
.Open "GET", sUrl, False
.send
sContent = .responseText
End With
'This retrieves values of currency (until end with)
With CreateObject("VBScript.RegExp")
.Global = True
.MultiLine = True
.IgnoreCase = False
.Pattern = "<span class=""avgMonth"">(.*?)</span>"
'To do the count, you must always execute the regex first
Set intMatches = .Execute(sContent)
If intMatches.Count <> 0 Then
With intMatches
For mtchCnt2 = 0 To .Count - 1
GetSingleMonth = .Item(mtchCnt2).SubMatches(0)
Cells(c, d).Value = GetSingleMonth
c = c + 1
Next
End With
End If
End With
End Function
发布于 2016-05-12 14:32:04
对你的问题做得很好。
很好的做法是缩进所有的代码,这样Labels
就会表现得非常明显。
Dim sUrl, sContent, intMatches
当您不定义变量时,VBA将将其声明为变量,即对象:
性能。使用对象类型声明的变量足够灵活,可以包含对任何对象的引用。但是,当您对这样一个变量调用方法或属性时,总是会导致延迟绑定(在运行时)。为了强制早期绑定(在编译时)和更好的性能,可以使用特定的类名声明变量,或者将其转换为特定的数据类型。
如果不声明变量,则可能要付出代价。
Dim a As Integer
Dim b As Integer
Dim c As Integer
Dim d As Integer
Dim i As Integer
Dim boolCtrl As Boolean
Dim period As Variant
Dim sCrcy As Variant
Dim MsgErr As String
变量名-给变量有意义的名称。
整数- 整数是过时的.根据msdn,VBA静默地将所有整数转换为long
。
boolCtrl - no need for bool
匈牙利命名?标准VBA命名约定的局部变量有camelCase
,其他变量和名称有PascalCase
。
With Selection
.HorizontalAlignment = xlCenter
.NumberFormat = "@"
End With
一定要避免像.Select
这样的事情--它只会通过在幕后处理其他所有事情时摆弄电子表格来减缓代码的速度。StackOverflow有一个很好的问题来解决这个问题-- https://stackoverflow.com/questions/10714251/how-to-avoid-using-select-in-excel-vba-macros。
'This retrieves values of currency (until end with)
评论- 代码告诉你怎么做,评论告诉你为什么。代码应该自己说明,如果需要注释,可能需要更清楚地说明。如果没有,评论应该描述你为什么要做某事,而不是你是怎么做的。这里有一个几个原因,以避免所有的评论在一起。
Function GetRateYear(sFromCrcy, sToCrcy, sYear, a, b)
如果可能的话,您应该传递参数ByVal而不是ByRef。ByRef是默认的。
您的函数应该是Private
而不是Public
。公开是默认的。
函数在返回时应该使用,在发生事情时应该使用subs。
Private Function GetRateYear(ByVal fromCurrency as String, ByVal toCurrency as String, ByVal year as String, ByVal a as Long, ByVal b as Long) As ??
发布于 2016-05-12 14:50:44
吹毛求疵,但是Sub fetchCurrencyPast()
应该是Public Sub FetchCurrencyPast()
,也就是使用与其他模块成员一致的PascalCasing,并且为了清晰而显式地使用Public
;VBA在背后做了很多事情,在可能的情况下总是显式地使用PascalCasing是很好的!
该过程中的第一个可执行语句作出了一个重要假设:
Columns("A:F").Select
Columns
调用是不合格的,这意味着它在代码运行时对任何工作表都是活动的。更糟糕的是,过程中的每个工作表访问语句都隐含地引用了活动工作表,这意味着如果用户在代码运行时激活了另一个工作表,则宏将开始输出到该新激活的工作表!
如果您总是关闭Sheet1
,那么您可以使用该对象引用来限定这些调用,这样无论用户在运行代码时做了什么,它都将始终使用相同的Sheet1
对象引用:
Sheet1.Columns("A:F").Select
理想情况下,您应该给该工作表一个有意义的名称,例如,如果它的选项卡名为"Results",您可以将工作表的代码名设置为ResultSheet
,并在代码中引用它:
ResultSheet.Columns("A:F").Select
现在,Select
、Activate
和使用Selection
并不是处理工作表的特别有效的方法。这通常是经验较少的VBA程序员所做的,因为这就是宏记录器的工作方式。
为什么我们在这里选择A:F列?
'Apply format text on cells, and centre it.
'Change format to text
Columns("A:F").Select
With Selection
.HorizontalAlignment = xlCenter
.NumberFormat = "@"
End With
这可以抽象为自己的程序:
Private Sub FormatResultSheet()
Dim target As Range
Set target = ResultSheet.Range("A:F")
target.HorizontalAligment = xlCenter
target.NumberFormat = "@" 'otherwise Excel trims leading zeroes
End Sub
说明代码正在做什么的注释是无用的,应该删除。唯一值得输入的注释是注释,说明代码为什么要做它所做的事情(参见上面的示例)。
这根本不需要:
'Clear selection
Cells(1, 1).Select
您的代码不应该被当前的选择所困扰。
我不想重复@Raystafarian的答复中关于这个块的精彩之处:
'Define variables
Dim a As Integer
Dim b As Integer
Dim c As Integer
Dim d As Integer
Dim i As Integer
Dim boolCtrl As Boolean
Dim period As Variant
Dim sCrcy As Variant
Dim MsgErr As String
但我要加上我的:
Chr(13)
为您提供了Windows用来编码新行的两个字符之一。另一个字符是Chr(10)
。
.." & Chr(13) & "..
一个更可移植的方法(你知道,如果你想在Mac上运行这个宏),并且通常是更易读的方法,就是使用内置的vbNewLine
常量,它生成操作系统喜欢使用的任何“新行”字符。
我读得越多,你就会越多地用注释来表示“这段代码做X":
‘'Add标头范围(“A1”,"F1").Style =“输入”范围(“A1”,"F1").Font.Bold =真实单元格(1,1).Value =“年”单元格(1,2).Value = "OffSetCurr“单元格(1,3).Value =”每月“单元格(1,4).Value = "toEuro”单元格(1,5).Value = "toDollars“单元格(1,6).Value = "toPounds”
每次你有评论说“这块钱做了(某件事)”,你就错过了一个机会(但永远不会太晚!)提取一种私人的方法来做这件事。
Private Sub AddHeaders()
With ResultSheet
.Range("A1", "F1").Style = "Input"
.Range("A1", "F1").Font.Bold = True
.Cells(1, 1).Value = "Year"
.Cells(1, 2).Value = "OffSetCurr"
.Cells(1, 3).Value = "Month"
.Cells(1, 4).Value = "toEuro"
.Cells(1, 5).Value = "toDollars"
.Cells(1, 6).Value = "toPounds"
End With
End Sub
https://codereview.stackexchange.com/questions/128160
复制相似问题