Posting accounting data to a report

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

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.

StackExchange Code Review Q#152791, answer score: 8