Transferring data for a given month from one sheet to another

Question

I have dollar amounts sorted by month that I want to transfer from sheet entry to sheet actuals. I only want to do it for a specified month prompted from user. Each dollar amount has a corresponding cost center/ID and description in separate columns. I want to copy the amounts and description from entry to the row in actuals matching the same ID.

Sometimes, multiple amounts share the same ID so I need to place them one below the other.

Sub month()

Dim actualsWS As Worksheet
Dim dataWS As Worksheet
Dim answer As String
Dim month As String
Dim loc As Range
Dim start As Integer
Dim rowCol As Variant
Dim dropRow As Integer
Dim dropCol As Integer
Dim locActual As Range

'store worksheets into variables
Set actualsWS = ThisWorkbook.Worksheets("Actuals by Month")
Set dataWS = ThisWorkbook.Worksheets("FAS Data Entry")

MsgBox ("If macro dies, it's likely a missing cost center.") 'warning for user

answer = InputBox(prompt:="What month?") 'prompt user for current month

'set month from user input
Select Case answer

    Case 1
    month = "January"
    'dropCol = 9
    Case 2
    month = "February"
    'dropCol = 10
    Case 3
    month = "March"
    'dropCol = 11
    Case 4
    month = "April"
    'dropCol = 12
    Case 5
    month = "May"
    'dropCol = 13
    Case 6
    month = "June"
    'dropCol = 14
    Case 7
    month = "July"
    'dropCol = 15
    Case 8
    month = "August"
    'dropCol = 16
    Case 9
    month = "September"
    'dropCol = 17
    Case 10
    month = "October"
    'dropCol = 18
    Case 11
    month = "November"
    'dropCol = 19
    Case 12
    month = "December"
    'dropCol = 20
    Case Else
    MsgBox ("Bad month. Exiting...")
    Exit Sub

End Select

Dim ccArray(99) 'cost center
Dim amountArray(99) As Currency 'amount
Dim descriptArray(99) 'description
Dim ccArrayLength As Integer

With dataWS

    'locate starting range for current month
    Set loc = .Cells.Find(month)

    'check previous row for carried over month (revising asset) Will break if 2 or more
    If loc.Offset(-1, 0) = "" Then 'if previous row is blank
        start = loc.row 'starting row is same row as found location above
    Else
        start = loc.row - 1 'else it's 1 row above
    End If

    i = 0

    'store data into arrays
    Do While .Cells(start + i, 1) <> "" 'loop through column A while there is a month present (not blank)

        ccArray(i) = .Cells(start + i, 11).Value 'store cost center
        amountArray(i) = .Cells(start + i, 13).Value 'store amount
        descriptArray(i) = .Cells(start + i, 9).Value 'store description

        i = i + 1

    Loop

End With

ccArrayLength = Application.Count(ccArray) 'store number of items

With actualsWS

    .Columns("V:W").ClearContents 'clear previous data

    'hide unnecessary months
    If month = "January" Or month = "February" Or month = "March" Or month = "April" Then
        .Columns("M:Q").EntireColumn.Hidden = True
    End If

    'drop data into respective cells onto "actuals by month" tab
    For x = 0 To ccArrayLength - 1

        On Error Resume Next
        Set locActual = .Cells.Find(ccArray(x)) 'find cost center location
        If locActual = 0 Then GoTo died 'not found
        'Debug.Print locActual
        'rowCol = locActual.Address
        dropRow = .Range(locActual.Address).row 'store row of cost center

        'for loop to find first empty cell to drop into
        For y = 0 To 99 'no cost center should have over 99 rows

            If .Cells(dropRow + y, 22).Value = "" Then 'column v blank
                .Cells(dropRow + y, 22).Value = amountArray(x) 'drop amount
                .Cells(dropRow + y, 22).Offset(0, 1).Value = descriptArray(x) 'drop description on row below
                Exit For
            End If

        Next

    Next

    .Cells(8, 22).Select 'bring cursor to top

End With

died:
'quit before msgbox if on last item
If x = ccArrayLength Then Exit Sub

MsgBox ccArray(x) & " not found." 'cost center not found
Exit Sub

End Sub

Answer 1

Sub month()

Dim month As String

Something isn't right, one of these two has the wrong name. Procedure names should start with a verb, they do something. If you can name that something, then you can name the procedure. If you can't name that something, then your procedure is doing too many things and needs to be broken down into something that can be named.

So we're getting the month name for some user input - if that's always going to be in English, then there's no need to hard-code that in a Select Case block.

Public Function ToMonthName(ByVal value As Integer) As String
    On Error GoTo CleanFail 'remove if runtime error 5 should bubble up to caller instead

    Dim result As String
    result = MonthName(value)

CleanExit:
    ToMonthName = result
    Exit Function
CleanFail:
    result = vbNullString
    Resume CleanExit
End Function

So, now we have the month name (or an empty string), that month procedure can actually start focusing on its task.

It's confusing that every code paths execute the died: label. There's no error handling - seeing that line label I was expecting to see On Error GoTo died somewhere.

So two things are happening here. First we're iterating dataWS and populating three arrays in parallel - there's a design smell right here: you have 3 data structures that are "in sync", when there should really be only one.

Make a small class module, and expose the 3 properties you're interested in:

Option Explicit
Private Type TModel
    CostCenter As String
    Description As String
    Amount As Single
End Type

Private this As TModel

Public Property Get CostCenter() As String
    CostCenter = this.CostCenter
End Property

Public Property Let CostCenter(ByVal value As String)
    this.CostCenter = value
End Property

Public Property Get Description() As String
    Description = this.Description
End Property

Public Property Let Description(ByVal value as String)
    this.Description = value
End Property

Public Property Get Amount() As Single
    Amount = this.Amount
End Property

Public Property Let Amount(ByVal value As Single)
    this.Amount = value
End Property

Now you can store instances of this class, which encapsulates the data you're interested in. This:

Do While .Cells(start + i, 1) <> "" 'loop through column A while there is a month present (not blank)

    ccArray(i) = .Cells(start + i, 11).Value 'store cost center
    amountArray(i) = .Cells(start + i, 13).Value 'store amount
    descriptArray(i) = .Cells(start + i, 9).Value 'store description

    i = i + 1

Loop

Becomes this:

Dim data As Collection
Set data = New Collection

Dim item As DataModel 'assuming class name was "DataModel"

Do While .Cells(start + i, 1) <> vbNullString 'loop through column A while there is a month present (not blank)

    Set item = New DataModel
    item.CostCenter = .Cells(start + i, 11).Value
    item.Amount = .Cells(start + i, 13).Value
    item.Description = .Cells(start + i, 9).Value

    data.Add item        
    i = i + 1

Loop

And now if the body of that loop was extracted into its own function...

Private Function ReadModel(ByVal source As Range) As DataModel
    Dim result As New DataModel
    result.CostCenter = source.Cells(1, 11).Value
    result.Amount = source.Cells(1, 13).Value
    result.Description = source.Cells(1, 9).Value
    Set ReadModel = result
End Function

You could also define an Enum to get rid of the magic values there:

Private Enum ColumnPosition
    DescriptionColumn = 9
    CostCenterColumn = 11
    AmountColumn = 13
End Enum

And then use them to enhance readability here:

Private Function ReadModel(ByVal source As Range) As DataModel
    Dim result As New DataModel
    result.CostCenter = source.Cells(1, CostCenterColumn).Value
    result.Amount = source.Cells(1, AmountColumn).Value
    result.Description = source.Cells(1, DescriptionColumn).Value
    Set ReadModel = result
End Function

The calling code will now look like this:

Dim data As Collection
Set data = New Collection

Do While .Cells(start + i, 1) <> vbNullString 'loop through column A while there is a month present (not blank)

    data.Add ReadModel(.Range("A" & start + i).EntireRow)
    i = i + 1

Loop

And now if you ever need to read more values, that code doesn't even need to change - you can simply modify the DataModel accordingly, and the function that reads a row's values into an instance. And this loop and collection should go into its own function, too.

The number of items in the collection would be simply data.Count.

Next the loop that consumes this data, could be a For Each loop:

Dim item As DataModel
For Each item In data
    '...
Next

Again, this should be in its own procedure. On Error Resume Next shouldn't need to be there. What errors are being thrown under the carpet? They should be handled.

Answer 2

Option Explicit

If that's not already at the top of your module, go put it there, then get it to insert automatically by going to Tools --> Options --> Require Variable Declaration. By forcing you to declare all your variables, this will prevent a ton of bugs by catching, among other things, typos and incompatible data types.

---

Codenames

You can give a sheet a codename by going to the Properties window in the VBE, selecting your worksheet and setting the (name) property. A codename is, effectively, a globally-scoped variable that refers to the worksheet. So, if you gave your "Actuals by Month" sheet the codename actualsSheet, rather than doing this:

Dim actualsWS As Worksheet
Set actualsWS = ThisWorkbook.Worksheets("Actuals by Month")
actualsWs.Columns("V:W").ClearContents

You can simply go:

actualsSheet.Columns("V:W").ClearContents

And now, if the user changes the name of your worksheet, the first set of code will fail, but the second set will keep carrying on as normal.

---

User Warnings

MsgBox ("If macro dies, it's likely a missing cost center.") 'warning for user

I appreciate the intent, I really do, but this is terrible for 2 reasons.

---

Reason #1: It Destroys any confidence the user has in your code.

If I'm a user and I see that, I think "Why the hell has this guy left a known bug in the application!", then "Wait, probably!? what else is going to go wrong here".

---

Reason #2: If there's a known bug then you should fix it.

This is inexcusable. If there's a known bug in your code then you fix it. And if you can't fix it, then you leave detailed instructions on how to avoid it, and how to fix it. And rather than giving the user a warning every time they run your program, you just set up error handling to tell them, if it occurs, what happened. Even better, set up error handling to complete the program anyway, then tell the user that you did X and Y but couldn't do Z because the cost center was missing.

---

Naming

Naming is one of the most important aspects of programming.

Quick question: How useful is your code?

You might think "Well, it probably depends on the problem it solves". But that's not what's useful. What's useful is code that other people can understand. And that includes future you. Your code could literally cure cancer, but if nobody can understand it, then nobody can use it.

So, your code should be written in a way that makes it easy for a complete stranger to figure out what it's doing and why.

The most effective way of doing this is proper, descriptive, unambiguous, concise, naming. I highly recommend the Excellent, Classic article on Naming by Joel Spolsky (In fact I recommend reading the entire blog, but start there).

---

Consider actualsWS. Now, this is sorta-useful. It quite clearly corresponds to "Actuals by Month" sheet. But, we don't really care what it's called in the code. What we care is that it is where we're copying our information.

Which is clearer about what's going on?

actualsWS.Columns("V:W").ClearContents

or

Dim destinationSheet As Worksheet
Set destinationSheet = actualsWS

destinationSheet.Columns("V:W").ClearContents

---

How about answer? Again, kinda-useful. It sorta implies there's a question somewhere, but what question? what answer? Very ambiguous. userInputMonthIndex is a little longer, but is so much clearer. I can see that variable anywhere later on in the code and know exactly what it is. Screen real-estate is cheap and plentiful. Cognitive processing is not. Optimise for ease of comprehension.

And then month. Is that a month number? or maybe a month name? or maybe some other month-object? You might consider just lugging your monthIndex around the code and calling MonthName( monthIndex ) whenever you need the name. If not, then give it a clear name. Something like inputMonthName (monthName would be better but it's already the name of a Function, so it would conflict).

Answer 3

As a user, I've always been annoyed with an app that does not allow me to cancel an action or back out of the app gracefully. As a developer, I've always felt the responsibility to present these options to the user -- it makes them feel all warm and fuzzy and believe that my app has a "polish" and quality to it. So I've built up a set of quick-use input forms I can adapt to the situation.

Your situation above where you ask the user for a starting month is a case in point. I'd rather see an input box with an "OK" and a "Cancel" button, where if the user changes his mind (or doesn't know the answer) can gracefully back out. Excel's standard InputBox does not provide this flexibility.

The userform here gives the user exactly that option, with very little code to implement it.

In the VBAProject as a Form called StartDateForm:

And the StartDateForm code:

Option Explicit

Private lastUserCmd As String

Public Property Get Month()
    Month = Me.cbMonth.Value
End Property

Public Property Get Year()
    Year = Me.cbYear.Value
End Property

Public Property Get UserAction()
    UserAction = lastUserCmd
End Property

Private Sub UserForm_Initialize()
    '--- set up the combo boxes
    Me.cbMonth.AddItem "Jan"
    Me.cbMonth.AddItem "Feb"
    Me.cbMonth.AddItem "Mar"
    Me.cbMonth.AddItem "Apr"
    Me.cbMonth.AddItem "May"
    Me.cbMonth.AddItem "Jun"
    Me.cbMonth.AddItem "Jul"
    Me.cbMonth.AddItem "Aug"
    Me.cbMonth.AddItem "Sep"
    Me.cbMonth.AddItem "Oct"
    Me.cbMonth.AddItem "Nov"
    Me.cbMonth.AddItem "Dec"
    Me.cbYear.AddItem "2015"
    Me.cbYear.AddItem "2016"
    Me.cbYear.AddItem "2017"
    Me.cbYear.AddItem "2018"

    '--- set default
    Me.cbMonth.ListIndex = 0
    Me.cbYear.ListIndex = 1

    lastUserCmd = "Cancel"

    Me.Show
End Sub

Private Sub cmdCancel_Click()
    lastUserCmd = "Cancel"
    Me.Hide
End Sub

Private Sub cmdOk_Click()
    lastUserCmd = "Ok"
    Me.Hide
End Sub

It's very easy to use, as in this test sub:

Option Explicit

Sub test()
    Dim userInputForm As StartDateForm

    Set userInputForm = New StartDateForm
    Do Until userInputForm.Visible = False
        DoEvents
    Loop
    If userInputForm.UserAction = "Ok" Then
        Debug.Print "User pressed OK - ";
        Debug.Print "Date selected is " & userInputForm.Month & "-" & userInputForm.Year
    Else
        Debug.Print "User cancelled input"
    End If
End Sub