# Transferring data for a given month from one sheet to another

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

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
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

Exit Sub

End Sub


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

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)

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.

• Wow, nothing like typing item and have intellisense popup ^___^ I feel the functions are a bit overkill but definitely appreciate the OOP design. It feels intelligent^^ And it does lend to growth later. This has been really helpful in learning/using class. Thank you so much <3 Jun 20 '16 at 20:32
• @findwindow you mean the Set locActual = .Cells.Find(ccArray(x)) part? Aye, I noticed, but didn't have time to dig deeper into it and think of a better way - indeed, that's certainly a bottleneck! as for functions being overkill, remember that programming isn't about writing something that works - any "script" can do that; programming is about making abstractions. The more you can abstract "things to do" into their own little box, the less you'll search for things that break, because when something breaks you'll know exactly where it is - whether you know the code inside out or not! Jun 20 '16 at 20:37
• Programming is 20% writing code, 80% reading code =) Jun 20 '16 at 20:42
• Also, I am in awe that you're so generous to provide world class programming lessons to me/random strangers! CR is a whole different level than SO^^ Jun 20 '16 at 20:43
• Yep, you're starting to see what brought me here now! Jun 20 '16 at 20:45

## 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).

• Option explicit is annoying when I just want to use counters for loop XD So I don't need to declare/set worksheets if I use codename?! Edit: so I can just do with on the codename as well? Jun 20 '16 at 16:43
• Yes, yes you can ^^ and declare your counters! Declare everything!.
– Kaz
Jun 20 '16 at 16:48
• Yep. Future me has strangled past me so many times I wonder how I'm still alive. Jun 20 '16 at 16:55
• @findwindow Depends. If you only declare i and j and accidentally put k instead, then Option Explicit will absolutely catch it.
– Kaz
Jun 20 '16 at 16:56
• This is also a good argument for naming your counters!
– Kaz
Jun 20 '16 at 16:58

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

'--- 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

• Way overkill for my purpose but appreciate another example of class. Jun 22 '16 at 18:21