6
\$\begingroup\$

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
asked Jan 18, 2016 at 13:39
\$\endgroup\$
2
  • \$\begingroup\$ Where does ws from the call of UnhideWsCellsAndRemoveFilters ws in the GetWsDataRange() come from ? \$\endgroup\$ Commented Jan 18, 2016 at 14:11
  • \$\begingroup\$ @Heslacher Ah yes, typo. Thanks for spotting that ^^ \$\endgroup\$ Commented Jan 18, 2016 at 15:13

1 Answer 1

5
\$\begingroup\$

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 
answered Jan 18, 2016 at 14:23
\$\endgroup\$
1
  • \$\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\$ Commented Jan 18, 2016 at 15:27

Your Answer

Draft saved
Draft discarded

Sign up or log in

Sign up using Google
Sign up using Email and Password

Post as a guest

Required, but never shown

Post as a guest

Required, but never shown

By clicking "Post Your Answer", you agree to our terms of service and acknowledge you have read our privacy policy.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.