4
\$\begingroup\$

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
\$\endgroup\$
7
\$\begingroup\$

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.

\$\endgroup\$
  • 1
    \$\begingroup\$ I understand exactly what you are saying regarding the comments...it may make a difference to know that I'm a contractor writing code for non-programmers to try to maintain (minimally) without my full time presence. I admit that many of the above comments are redundant based on the code itself...but this format seems to be what the client wants. The previous employee that wrote their reports was an information hoarder and made the code extremely more difficult than was needed because he thought that would mean job security. There were almost 0 comments when I started. \$\endgroup\$ – Rdster Jan 16 '17 at 19:39
  • 3
    \$\begingroup\$ IMO writing code for non-programmers to maintain is an even stronger incentive to show how it's done right - and write simple, self-explanatory code with meaningful identifiers and descriptive procedure names. If someone reads For Each sheet In ActiveWorkbook.Worksheets and doesn't pick up that it's iterating all sheets in the active workbook, I'm not sure a comment that says exactly that is going to help them anyway ;-) \$\endgroup\$ – Mathieu Guindon Jan 16 '17 at 20:16
  • 2
    \$\begingroup\$ I would take your advice here one step further: you should pretty much always use Long unless you actually need a 16-bit integer. A 32-bit type is going to be faster, and unless you're packing a bunch of them into a structure, will either use the same amount of memory or not enough additional memory to matter. So you effectively get the additional capacity and future-proofing for free. \$\endgroup\$ – Cody Gray Jan 17 '17 at 11:06
  • 2
    \$\begingroup\$ "These prefixes are an incarnation of the bad kind of Hungarian Notation and should be killed with fire." THANK YOU sir for this link. I was always told to use the Hungarian notation and always used in every single code of mine. Now I will remember this article every time I name my variables and functions \$\endgroup\$ – Victor Moraes Jan 17 '17 at 12:24
5
\$\begingroup\$

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

\$\endgroup\$
  • 1
    \$\begingroup\$ Another improvement might be to pass the Worksheet as parameter to the sub also. That would make the code more generic and flexible to use. \$\endgroup\$ – πάντα ῥεῖ Jan 16 '17 at 18:04
  • \$\begingroup\$ True that and I was about to add that comment myself \$\endgroup\$ – Victor Moraes Jan 16 '17 at 18:05
  • 1
    \$\begingroup\$ Agreed, however, I'm using the Find to determine if the sheet should be skipped...so I wouldn't really need to to pass it in this instance. \$\endgroup\$ – Rdster Jan 16 '17 at 18:15

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.