4
\$\begingroup\$

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 in Data G2 aka. range "FindDate"
                    End With
                With Turnover2017.Range("A:AM")      '*************** Edit Year Number here upon creation of next year's tab
                    Set NTRNG = .Find(What:=(FindDate), _
                        After:=Range("A18"), _
                        LookIn:=xlFormulas, _
                        LookAt:=xlPart, _
                        SearchOrder:=xlByRows, _
                        SearchDirection:=xlNext, _
                        MatchCase:=False, _
                        SearchFormat:=False)
                            NTRNG.Offset(X, 1).Cells.Value = NTNUM     '******** This updates the appropriate column based on the date determined in Data G2 aka. range "FindDate"
                    End With
                With Turnover2017.Range("A:AM")      '*************** Edit Year Number here upon creation of next year's tab
                    Set PTRNG = .Find(What:=(FindDate), _
                        After:=Range("A1"), _
                        LookIn:=xlFormulas, _
                        LookAt:=xlPart, _
                        SearchOrder:=xlByColumns, _
                        SearchDirection:=xlNext, _
                        MatchCase:=False, _
                        SearchFormat:=False)
                            PTRNG.Offset(X, 1).Cells.Value = PTNUM    '******** This updates the appropriate column based on the date determined in Data G2 aka. range "FindDate"

                     End With

        Next X


    End Sub
\$\endgroup\$
1
  • \$\begingroup\$ What is your question? Do you have any issues with it? Or do you just want to get best practices / tips? \$\endgroup\$ Commented Feb 2, 2017 at 23:38

1 Answer 1

Reset to default
2
\$\begingroup\$

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

  1. 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.
  2. 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.
  3. 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.
  4. 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.
  5. Explicitely declare the scope of procedures and variables, i.e. declare them as Private, Public or Friend.
  6. 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.
  7. 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

  1. 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.
  2. 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.
  3. 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.
  4. 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.
  5. 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.
  6. 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.)
  7. 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.
  8. 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.
\$\endgroup\$

You must log in to answer this question.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.