Populating monthly report with new data each month

This code basically checks a cell for the current date and then fills out the report by finding that date and placing the correct values in the correct places. It works fine I just know that I am going to need to build something similar for many other reports so I am hoping to update, streamline and improve in any way I can.

```
Sub Copy_Data()
Dim Data As Worksheet
Set Data = ThisWorkbook.Worksheets("Data")
Dim Turnover2017 As Worksheet
Set Turnover2017 = ThisWorkbook.Worksheets("2017 Turnover") '* Edit Year Number here upon creation of next year's tab
Dim FindDate As Date
FindDate = Data.Range("FindDate").Value
Dim HCRNG As Range
Dim NTRNG As Range
Dim PTRNG As Range
Dim X As Integer
Dim HCNUM As Integer
Dim NTNUM As Integer
Dim PTNUM As Integer

For X = 1 To 11
'*** This block scoops data from the Data tab
HCNUM = Data.Range("B2").Offset(X, 0).Cells.Value
NTNUM = Data.Range("C2").Offset(X, 0).Cells.Value
PTNUM = Data.Range("D2").Offset(X, 0).Cells.Value
With Turnover2017.Range("A:AM") '* Edit Year Number here upon creation of next year's tab
Set HCRNG = .Find(What:=(FindDate), _
After:=Range("A1"), _
LookIn:=xlFormulas, _
LookAt:=xlPart, _
SearchOrder:=xlByRows, _
SearchDirection:=xlNext, _
MatchCase:=False, _
SearchFormat:=False)
HCRNG.Offset(X, 2).Cells.Value = HCNUM '**** This updates the appropriate column based on the date determined in Data G2 aka. range "FindDate"
HCRNG.Offset(X + 15, 2).Cells.Value = HCNUM '**** This updates the appropriate column based on the date determined

Let me start with a number of general guidelines that can make your code more maintainable and more reusable. After that I will give some more specific advise regarding your code.

General Guidelines

• Good variable names are one of the best ways to document what your code is doing. If someone else ever reads your code, he will be very grateful in case the variable names actually tell him what the variables are. This might actually also apply to yourself coming back to code after a few month. In the case of FindDate you are already using a good name. However, I have absolutely no idea what PTNUM is.

• Try to follow the single responsibility principle. In short it states that a unit of your code, e.g. a procedure, should do one and only one thing. This leads to shorter more focused procedures that are easy to understand. Combined with good naming, this makes nearly all comments unneccesary. Admittedly, often it is rather hard to get this principle really right.

• Do not use magical numbers, like e.g. 1 and 11 in your code. Either declare a constants minVerticalOffset and maxVerticalOffset or even pass these as parameters.

• Do not hard-code configuration all over the place. Your comments show that you have to adjust the code in several places every year. Instead you should rather extract all but the configuration into a parametrized procedure that you call from Copy_Data.

• Explicitely declare the scope of procedures and variables, i.e. declare them as Private, Public or Friend.

• Declare you variables close to where you actually use them. I know that this contradicts with old coding conventions for VBA, but having the declaration right before its use makes it clear of what type the variable is without any need of a naming convention.

• Rather VBA specific, use Long in favor of Integer if you do not specifically want to limit the range to that of an integer. As explained in the answer to this SO question every Integer is internally converted to a Long anyway on any halfway modern system.

Specific Advice

• As I hinted at already above, the variable names in all caps could use some work. E.g. the variable X could be renamed to verticalOffset.

• Getting more specific regarding point 4 above, it might be of great use for reusability to extract the body of the procedure into its own (private) procedure with three parameters: TurnoverSheet As WorkSheet, FindDate As Date, DataSheet As worksheet. The names might need a little work, though. The parameters would then be passed in when calling the procedure from Copy_Data.

• Most probably, your code contains a hard to see bug. Inside the with blocks, the Range(AddressString) is not prefixed with a dot. Thus, it refers to the range on the active sheet. This means that this procedure only works when called from the sheet Turnover2017.

• Your three With blocks all refer to the same variable. Hence, they can be combined into one and even be pushed outside the For loop.

• When using Range.Find you should always consider the possibility that there is no search result. In this case Nothing is returned. This would lead to runtime errors whan trying to call anything on it. So, you might want to test the result range for Is Nothing.

• From your code I get the impression that you first only want to search in the first row, then only in the 18th row and then only in the first column. If this is the case, then you should actually use the appropriate ranges. (This would also make the use of the optioanl parameter SearchOrder obsolete, which contains the very subtle difference between the first and third search.)

• The performance could be improved by pushing the searches out of the For loop. Your searches are independent of the value of X. So, at the moment, you are performing the same work for each value of this variable. If you have really large worksheets, this wastefulness can be a problem.

• After pushing the searches out of the For loop, you can eliminate the loop alltogether. To do this wyou have to load the three sets of data each into a two dimensional array by assigning the Value property, or better Value2 property, of the the corresponding ranges to a Variant variable. (See this blog post for why to use Value2.) Then the values can be assigned to the target ranges by setting the Value property of the top left cell of the target range to the corresponding array. This is much faster then itterating over the cells because accessing the worksheet is rather slow.

StackExchange Code Review Q#154301, answer score: 2