I'm re-writing my module of Standard Methods. Virtually every project I do begins with grabbing some number of Data Tables and putting them in arrays. So, this is my general "Get Worksheet Data" method(s).
As always, particularly interested in maintainability, but all feedback is welcome.
Public Function GetWsDataArray(ByRef wbTarget As Workbook, ByRef wsTarget As Worksheet, ByVal topLeftCellText As String, ByVal useCurrentRegion As Boolean _
, Optional ByVal searchStartRow As Long = 1, Optional ByVal searchStartColumn As Long = 1 _
, Optional ByVal searchEndRow As Long = 10, Optional ByVal searchEndColumn As Long = 10) As Variant
'/ 10x10 is arbitrary search range that should cover almost all typical worksheets
Dim dataArray As Variant
dataArray = Array()
dataArray = GetWsDataRange(wbTarget, wsTarget, topLeftCellText, useCurrentRegion, searchStartRow, searchStartColumn, searchEndRow, searchEndColumn)
GetWsDataArray = dataArray
End Function
Public Function GetWsDataRange(ByRef wbTarget As Workbook, ByRef wsTarget As Worksheet, ByVal topLeftCellText As String, ByVal useCurrentRegion As Boolean _
, ByVal searchStartRow As Long, ByVal searchStartColumn As Long _
, ByVal searchEndRow As Long, ByVal searchEndColumn As Long) As Range
Dim wbSource As Workbook, wsSource As Worksheet
Set wbSource = ActiveWorkbook
Set wsSource = ActiveSheet
wbTarget.Activate
wsTarget.Activate
UnhideWsCellsAndRemoveFilters wsTarget
Dim topLeftCell As Range, searchRange As Range, dataRange As Range
Set searchRange = wsTarget.Range(Cells(searchStartRow, searchStartColumn), Cells(searchEndRow, searchEndColumn))
Set topLeftCell = CellContainingStringInRange(searchRange, topLeftCellText)
Dim lastRow As Long, lastCol As Long
If useCurrentRegion Then
Set dataRange = topLeftCell.CurrentRegion
Else
lastRow = Cells(Rows.Count, topLeftCell.Column).End(xlUp).row
lastCol = Cells(topLeftCell.row, Columns.Count).End(xlToLeft).Column
Set dataRange = wsTarget.Range(topLeftCell, Cells(lastRow, lastCol))
End If
Set GetWsDataRange = dataRange
wbSource.Activate
wsSource.Activate
End Function
Public Function CellContainingStringInRange(ByRef rngSearch As Range, ByVal strSearch As String) As Range
Dim errorMessage As String
Set CellContainingStringInRange = rngSearch.Find(strSearch, LookIn:=xlValues)
If CellContainingStringInRange Is Nothing _
Then
errorMessage = "Couldn't find cell """ & strSearch & """ in " & rngSearch.Worksheet.name
PrintErrorMessage errorMessage, stopExecution:=True
End If
End Function
Public Sub UnhideWsCellsAndRemoveFilters(ByRef ws As Worksheet)
ws.Rows.Hidden = False
ws.Columns.Hidden = False
ws.AutoFilterMode = False
End Sub
1 Answer 1
UnhideWsCellsAndRemoveFilters()
Well thats a real strange name. Wouldn't it be better to replace Unhide
with Show
? Like ShowWsCellsAndRemoveFilters()
?
CellContainingStringInRange()
You should declare your variables as near as possible to their usage. This together with reverting the if
condition to return early would lead to
Public Function CellContainingStringInRange(ByRef rngSearch As Range, ByVal strSearch As String) As Range
Set CellContainingStringInRange = rngSearch.Find(strSearch, LookIn:=xlValues)
If CellContainingStringInRange IsNot Nothing Then
Exit Function
End If
Dim errorMessage As String
errorMessage = "Couldn't find cell """ & strSearch & """ in " & rngSearch.Worksheet.name
PrintErrorMessage errorMessage, stopExecution:=True
End Function
GetWsDataRange()
At first glance at this
Dim wbSource As Workbook, wsSource As Worksheet Set wbSource = ActiveWorkbook Set wsSource = ActiveSheet
it will be overseen that wbSource
and wsSource
are different objects. You should consider to use some vertical spacing to separate these and/or to use different names like sourceBook
and sourceSheet
. The same is true for the method arguments wbTarget
and wsTarget
.
Generally declaring multiple variables on the same line should be avoided because it is much harder to read/maintain.
Having the dataRange As Range
doesn't provide any value because it is only used "once". You can assign the return value directly.
Implementing this points and having the declarations as near to their usage as possible and on separate lines will lead to
Public Function GetWsDataRange(ByRef targetBook As Workbook, ByRef targetSheet As Worksheet, ByVal topLeftCellText As String, ByVal useCurrentRegion As Boolean _
, ByVal searchStartRow As Long, ByVal searchStartColumn As Long _
, ByVal searchEndRow As Long, ByVal searchEndColumn As Long) As Range
Dim sourceBook As Workbook
Set sourceBook = ActiveWorkbook
Dim sourceSheet As Worksheet
Set sourceSheet = ActiveSheet
targetBook.Activate
targetSheet.Activate
ShowWsCellsAndRemoveFilters ws
Dim searchRange As Range
Set searchRange = targetSheet.Range(Cells(searchStartRow, searchStartColumn), Cells(searchEndRow, searchEndColumn))
Dim topLeftCell As Range
Set topLeftCell = CellContainingStringInRange(searchRange, topLeftCellText)
Dim dataRange As Range
If useCurrentRegion Then
Set GetWsDataRange = topLeftCell.CurrentRegion
Else
Dim lastRow As Long
lastRow = Cells(Rows.Count, topLeftCell.Column).End(xlUp).row
Dim lastCol As Long
lastCol = Cells(topLeftCell.row, Columns.Count).End(xlToLeft).Column
Set GetWsDataRange = targetSheet.Range(topLeftCell, Cells(lastRow, lastCol))
End If
sourceBook.Activate
sourceSheet.Activate
End Function
-
\$\begingroup\$ Thanks, very useful stuff. Re: one-use variables, I like to use them because, IMO, it allows for greater descriptivity (is that a word?) and if I want to do other things with that variable in future, I won't have to go back and code it in. \$\endgroup\$Kaz– Kaz2016年01月18日 15:27:25 +00:00Commented Jan 18, 2016 at 15:27
ws
from the call ofUnhideWsCellsAndRemoveFilters ws
in theGetWsDataRange()
come from ? \$\endgroup\$