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

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

Not the answer you're looking for? Browse other questions tagged or ask your own question.