3
\$\begingroup\$

So I recently rebuilt a worksheet we have for tracking our clients who take income from their portfolios (here). I intend to be pulling this data into other workbooks for various reporting functions.

To facilitate this, I decided to write methods in the "workbook" module to return:

  • The codenames of the sheets holding data tables
  • The headers used in the data tables
  • The data table from a specific sheet (specified by codename)

Is this an effective way of achieving my goal (reducing potential problems interfacing between workbooks)?


Module "A1_Public_Variables"

Option Explicit
Public Const TOP_LEFT_CELL_STRING As String = "Client Name"
Public Const CLIENT_NAME_HEADER As String = "Client Name"
Public Const INCOME_AMOUNT_HEADER As String = "Income"
Public Const PAYMENT_FREQUENCY_HEADER As String = "Frequency"
Public Const PAYMENT_DAY_HEADER As String = "Date Paid"
Public Const BASE_MONTH_HEADER As String = "Base Month"
Public Const ASCENTRIC_WRAPPER_HEADER As String = "Wrapper"
Public Const ASCENTRIC_ACCOUNT_NUMBER_HEADER As String = "Ascentric Acc #"
Public Const ACCOUNT_TO_PAY_FROM_HEADER As String = "Account to pay from?"
Public Const WS_2015_CODENAME As String = "ws_2015"
Public Const WS_2016_CODENAME As String = "ws_2016"

"Workbook" Module

Option Explicit
Public Sub GetDataTableHeaders(Optional ByRef topLeftCellString As String, Optional ByRef clientNameHeader As String, Optional ByRef incomeAmountHeader As String _
 , Optional ByRef paymentFrequencyHeader As String, Optional ByRef paymentDayHeader As String, Optional ByRef baseMonthHeader As String _
 , Optional ByRef ascentricWrapperHeader As String, Optional ByRef ascentricAccountNumberHeader As String, Optional ByRef accountToPayFromHeader As String)
 topLeftCellString = CLIENT_NAME_HEADER
 clientNameHeader = CLIENT_NAME_HEADER
 incomeAmountHeader = INCOME_AMOUNT_HEADER
 paymentFrequencyHeader = PAYMENT_FREQUENCY_HEADER
 paymentDayHeader = PAYMENT_DAY_HEADER
 baseMonthHeader = BASE_MONTH_HEADER
 ascentricWrapperHeader = ASCENTRIC_WRAPPER_HEADER
 ascentricAccountNumberHeader = ASCENTRIC_ACCOUNT_NUMBER_HEADER
 accountToPayFromHeader = ACCOUNT_TO_PAY_FROM_HEADER
End Sub
Public Sub GetWorksheetCodenames(Optional ByRef ws2015 As String, Optional ByRef ws2016 As String)
 ws2015 = WS_2015_CODENAME
 ws2016 = WS_2016_CODENAME
End Sub
Public Function GetDataArrayFromSheetByCodename(ByVal strCodeName As String) As Variant
 '/ returns the datatable, or an error if could not find worksheet
 Dim dataTable As Variant
 dataTable = Array()
 Dim wsWasFound As Boolean
 Dim ws_target As Worksheet, ws As Worksheet
 wsWasFound = False
 For Each ws In ThisWorkbook.Worksheets
 If ws.codeName = strCodeName Then
 Set ws_target = ws
 wsWasFound = True
 End If
 Next ws
 If wsWasFound Then
 Dim tableRange As Range
 Set tableRange = GetTableRange(ws_target)
 dataTable = tableRange
 GetDataArrayFromSheetByCodename = dataTable
 Else
 GetDataArrayFromSheetByCodename = CVErr(2042) '/ #N/A error
 End If
End Function
asked Dec 9, 2015 at 11:52
\$\endgroup\$
8
  • \$\begingroup\$ What's with the method that does nothing but return constants ByRef through out params? Why not just directly reference the constants? \$\endgroup\$ Commented Dec 10, 2015 at 0:10
  • 2
    \$\begingroup\$ because it's designed to be called from another workbook. And I'd rather not have the other workbook assume anything about the internal structure of this one. \$\endgroup\$ Commented Dec 10, 2015 at 0:33
  • 1
    \$\begingroup\$ Still i don't understand what GetDataTableHeaders does... there is no output... is there a way to use a Sub in a way i don't know? I feel like i miss something really big \$\endgroup\$ Commented Dec 10, 2015 at 0:42
  • 1
    \$\begingroup\$ @DirkReichel he's passing the params By *Reference, which means that you can set the argument's value inside the sub, and the change will be visible from the caller. It's a neat trick, but generally frowned upon unless you've got a good reason. \$\endgroup\$ Commented Dec 10, 2015 at 0:56
  • \$\begingroup\$ In other words, there is output. It's just not obvious at a glance. \$\endgroup\$ Commented Dec 10, 2015 at 0:59

1 Answer 1

2
\$\begingroup\$

Just looking at your function there 2 ways to improve it:

Public Function GetDataArrayFromSheetByCodename(ByVal strCodeName As String) As Variant
 Dim ws As Worksheet
 For Each ws In ThisWorkbook.Worksheets
 If ws.CodeName = strCodeName Then
 Set GetDataArrayFromSheetByCodename = GetTableRange(ws)
 Exit Function
 End If
 Next ws
 GetDataArrayFromSheetByCodename = CVErr(2042) '/ #N/A error
End Function

When having LOTS of sheets, this may be faster, but uses an error:

Public Function GetDataArrayFromSheetByCodename(ByVal strCodeName As String) As Variant
 On Error Resume Next
 If Len(ThisWorkbook.VBProject.VBComponents(strCodeName).Name) = 0 Then
 GetDataArrayFromSheetByCodename = CVErr(2042)
 Else
 Set GetDataArrayFromSheetByCodename = GetTableRange(ThisWorkbook.VBProject.VBComponents(strCodeName))
 End If
End Function

Also as a quick-inprove just add Exit For after your wsWasFound = True to not loop all sheets that are left after you found a match.

For the second version of the function: I use the fact that errors inside of conditions inside of If-statements are always true when using On Error Resume Next. Len(....Name) = 0 Then normally NEVER can be true when there is a need of a name. But having an error inside will act like the whole statement is true. So it simply tries to catch the item. If it exists, it also has a name this way Len will be >0 and be false (now your calculation will be done), else (it doesn't exist) it will act like If is true and the = CVErr(2042) will be set.
(削除) For the Subs: they doesn't make sense to me. What do they do? As it looks to me, they do simply nothing. (削除ここまで)

Last hint: You may change the function to As Range. When set a variable to it, you later could simply check by If Variable Is Nothing then. So there would be no need for = CVErr(2042).

EDIT
just to show the way i would hand over the values (while im pretty sure you will not use it)

Public Function GetDataTableHeaders() As Variant
GetDataTableHeaders = Array("Client Name", "Client Name", "Income", "Frequency", _
 "Date Paid", "Base Month", "Wrapper", "Ascentric Acc #", "Account to pay from?")
End Function

then simply set a variable to it or directly use GetDataTableHeaders(2) to get "Income"... as it looks, you may want to output the values to a sheet as headers and this way it would be much easier. However, you want to make it "not understandable" in some way, and for that it will be better to stay as it is right now :)

answered Dec 9, 2015 at 17:54
\$\endgroup\$
6
  • \$\begingroup\$ <meta review> Your answer would be more clear if you didn't one line the method. That's a lot of code for a single line. <meta review /> \$\endgroup\$ Commented Dec 10, 2015 at 0:09
  • \$\begingroup\$ @RubberDuck changed it... still, it does the same like the code does in the question, so there shouldn't be anything the OP can't understand... \$\endgroup\$ Commented Dec 10, 2015 at 0:37
  • \$\begingroup\$ I understood it and I liked it. I'd also never write it on a single line like that, for the reasons @RubberDuck mentioned. \$\endgroup\$ Commented Dec 10, 2015 at 0:38
  • \$\begingroup\$ The reason I use individual variables for each header is because, if I want to find the "Payment Day" column, for instance. my way: I just call GetDataTableHeaders paymentDayHeader:= somevar. Whereas your way, I'd have to know, in advance, which index in the array corresponded to the heading I wanted, at which point I might as well just hardcode them all anyway. \$\endgroup\$ Commented Dec 10, 2015 at 2:33
  • \$\begingroup\$ Imagine if you were interacting with an API that returned stock data. And rather than typing GetData(MSFT), you had to type GetData(StockList(1231)) and *hope* that that list never got re-ordered in the future. \$\endgroup\$ Commented Dec 10, 2015 at 2:39

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.