Posting accounting data to a report

Question

I have some buttons on an Excel report. 2 in particular are for posting all Budget numbers to the report and posting the Actual numbers. I currently have 2 separate subs, one assigned to each button.

Public Sub PostAllActual()
  Dim sheet As Worksheet
  Dim rFind As Range

  For Each sheet In ActiveWorkbook.Worksheets                       'Cycle through all of the sheets
    If sheet.Name <> "Index" Then                                   'Skip the Index sheet
      'Search for the Index row in Col A
      Set rFind = sheet.Cells.Find("Index", [H2], xlValues, xlWhole, xlByColumns, xlNext)
      If rFind Is Nothing Then                                      'Did we find the Index row?
        'Not Found - ORatio sheet
      Else                                                          'Found
        PostNumbers sheet, 37, "I"                                  'Procedure to copy numbers to the YTD
      End If
    End If
  Next sheet                                                        'Move to the next sheet
  Beep                                                              'Auditory notification of completed process
End Sub

Public Sub PostAllBudgets()
  Dim sheet As Worksheet
  Dim rFind As Range

  For Each sheet In ActiveWorkbook.Worksheets                       'Cycle through all of the sheets
    If sheet.Name <> "Index" Then                                   'Skip the Index Sheet
      'Search for the Index row in Col A
      Set rFind = sheet.Cells.Find("Index", [H2], xlValues, xlWhole, xlByColumns, xlNext)
      If rFind Is Nothing Then                                      'Did we find the Index row?
        'Not Found - ORatio sheet
      Else                                                          'Found
        PostNumbers sheet, 21, "H"                                  'Procedure to copy the numbers to the YTD
      End If
    End If
  Next sheet                                                        'Move to the next sheet
  Beep                                                              'Auditory notification of completed process
End Sub

Do I really save anything by reducing these to simple sub calls on the button click, and passing in the 2 parameters like this?

Public Sub PostAllBudgets()
  PostAllNumbers 21, "H"
End Sub

Public Sub PostAllActual()
  PostAllNumbers 37, "I"
End Sub

Public Sub PostAllNumbers(ByVal iRow as Integer, ByVal sCol as String)
  Dim sheet As Worksheet
  Dim rFind As Range

  For Each sheet In ActiveWorkbook.Worksheets                       'Cycle through all of the sheets
    If sheet.Name <> "Index" Then                                   'Skip the Index Sheet
      'Search for the Index row in Col A
      Set rFind = sheet.Cells.Find("Index", [H2], xlValues, xlWhole, xlByColumns, xlNext)
      If rFind Is Nothing Then                                      'Did we find the Index row?
        'Not Found - ORatio sheet
      Else                                                          'Found
        PostNumbers sheet, iRow, sCol                                  'Procedure to copy the numbers to the YTD
      End If
    End If
  Next sheet                                                        'Move to the next sheet
  Beep                                                              'Auditory notification of completed process
End Sub

Answer 1

Your indentation is consistent, but non-standard. Someone else inheriting that code might not be bothered with lining up all the end-of-line comments in the same column, and if their editor sets indent spaces to 4 instead of 2 they're likely going to turn it into a bit of a mess.

Unless they're using Smart Indenter or Rubberduck to automate the indentation. If you are not using a tool to achieve that indentation, then you're wasting your time lining up all these comments.

But are they good comments worth keeping around?

• Comments are supposed to make plain what the code does not tell us already.

• Good code seldom needs comments.

• Good comments say why, not what.

https://codereview.stackexchange.com/a/90113/23788

From what I can see, every single comment is merely rephrasing the code and brings absolutely nothing to the table. If I were maintaining that code, I'd wipe them all out. All of them.

There are a number of other issues with the code:

Set rFind = sheet.Cells.Find("Index", [H2], xlValues, xlWhole, xlByColumns, xlNext)

[H2] is evaluated off the active worksheet, which may not be the sheet that's being iterated. That makes the .Find call hard to parse, and makes me wonder if it works as it should. In 15 years I've never ever needed to use a bracketed expression anywhere. I'd replace it with sheet.Range("H2") to be safe.

I like that your Public members are explicitly public - because by default they're implicitly so. I also like that you're passing your parameters ByVal - especially since the default in VBA is an implicit ByRef.

iRow As Integer is potentially problematic: an Integer is a 16-bit signed integer type with a maximum value of 32,767... and an Excel worksheet can have many times more rows than that. Regardless of how many rows you think you need or can possibly deal with, you should always use Long instead of Integer, at least when referring to sheet rows.

I like how you used sheet as an identifier for a Worksheet object. I'm not sure what the r is doing in rRange, or what i does in iRow, or the s in sCol. These prefixes are an incarnation of the bad kind of Hungarian Notation and should be killed with fire.

• iRow As Integer could be destinationRowIndex As Long if that's its purpose (not clear from the posted code). Notice iRow As Long defeats the Hungarian prefix, and lRow As Long uses a lowercase l which can easily be confused for an 1 in the VBE's default font... which is uselessly confusing, since legal identifiers can't start with a digit in VBA.
• sCol As String could be destinationColumnHeading As String again if that's its purpose. Note that the distinction between Index and Heading reveals an implementation detail of your PostNumbers procedure; you're definitely using them to concatenate some Range address. Why not give it a single Range parameter then?
• rFind As Range could be findResult As Range. At least it wasn't an oRange... although, oranges are tasty ;-)

Note, these are suggestions - I'm not saying destinationColumnHeading is the ideal name to use here. What I am saying, is that an identifier should convey its purpose, not its type. The IDE tells you what the type is already.

---

I agree with your refactoring. What you've done here is called extracting a method, and it's a very good way to eliminate redundancies in code. Redundant code is bad for several reasons; the DRY (Don't Repeat Yourself) principle exists to remind us that it's always better to have one single place to make a change, than having multiple places to fix in order to make the same change - and run the risk of forgetting one and introducing bugs, as Victor's answer points out.

IMO, macros (procedures attached to some worksheet button) should be tiny little things, so I like what I'm seeing with these two one-liners that call a lower-abstraction procedure.

Answer 2

Regarding performance, I wouldn't see any improvements, but you will surely benefit from centralizing those subs into a parameterized Sub in terms of maintenance. It is much easier and faster to make changes in a single place, rather than in all duplicated code, so the last approach is preferable.
As greatly pointed out by πάντα ῥεῖ, you can also pass the Worksheet (and even the What parameter of the .Find) as parameter, to increase the flexibility and reusability