2
\$\begingroup\$

Background:

For several of my workbooks I run Overview sheets with charts in them. When printing the Overview sheet, I put my workbook name as header, where the workbook name includes the criteria for the generating the source data (e.g., date range and time stamp plus non-numeric strings).

To draw attention to these numbers/dates/times, I can manually highlight on the print out, but when others print (using a button on the Overview sheet) they don't seem to remember to do that.

I am looking to add some "flare" to my header by underlining the numeric values.


Issue:

The workbook name character length varies, so I assess character by character. In some cases the code executes after a delay (time varies, but will say that I had a 60 character length name take ~30 seconds), and other times Excel simply doesn't complete execution after several minutes (i will press escape and get the "code execution has been interupted" msgbox; this recently happened for a 48 character name).


Question:

As this is working code (sometimes), would you please help critique and possibly help resolve the time delay?

If my approach is inferior, I am open to suggestions, though I understand that, from my time on StackOverflow, subjective questions tend to be closed.


Code in question:

Option Explicit

Sub underline_numbers_in_header()
    Dim i As Long, n As String, z As String
    n = ActiveWorkbook.Name
    'Debug.Print Len(n)
    For i = 1 To Len(n)
        If IsNumeric(Mid(n, i, 1)) Then
            z = z & "&U" & Mid(n, i, 1) & "&U"
        Else
            z = z & Mid(n, i, 1)
        End If
        ActiveSheet.PageSetup.CenterHeader = "" & z & ""
    Next i
End Sub
\$\endgroup\$
  • \$\begingroup\$ "Code execution was interrupted" sounds like some ghost breakpoint - a VBE bug, nothing to do with your code. Try exporting the module and re-importing it again, then recompile the project. \$\endgroup\$ – Mathieu Guindon May 31 '19 at 15:18
2
\$\begingroup\$
ActiveSheet.PageSetup.CenterHeader = "" & z & ""

There's no reason to run this instruction at every single iteration as z is still in the process of being concatenated, is there?

Move it out of the loop and assign PageSetup.CenterHeader once, only when you've figured out the entire final string value - this could have a dramatically positive impact on performance :)

The name of the procedure/macro should not have underscores in it; convention is to use PascalCase for procedure/member names.

The procedure is implicitly Public and could use an explicit access modifier.

Reading the name of the procedure I was expecting it to go through page headers and underline whatever numeric values are in there... but that's not what's happening, and the variable names aren't helping much. Take the time to spell them out, and thank yourself later!

That said I'd suggest taking in a Workbook parameter and removing the ActiveWorkbook and ActiveSheet dependencies, which force any VBA code calling this procedure to Select and Activate things, which as you know isn't ideal.

Actually, the procedure has too many responsibilities, that's why its name feels wrong. It should be a function that takes a String argument and returns the formatted string: that's what I would expect a "make numbers underline" procedure to do. Then another procedure can be responsible for knowing what worksheet to interact with, and for setting its PageSetup.CenterHeader:

Dim headerText As String
headerText = UnderlineNumbers(ActiveWorkbook.Name)
ActiveSheet.PageSetup.CenterHeader = headerText

The string-valued Mid$ function should work slightly better than the similar but variant-valued Mid function, and when the current character is being pulled from the string in 3 places, it's time to introduce a local variable ;-)

This should be pretty much instant:

Private Function UnderlineNumbers(ByVal value As String) As String
    Dim result As String
    Dim i As Long
    For i = 1 To Len(value)
        Dim current As String
        current = Mid$(value, i, 1)
        If IsNumeric(current) Then
            result = result & "&U" & current & "&U"
        Else
            result = result & current
        End If
    Next
    UnderlineNumbers = result
End Function
| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ ...i left the append inside my loop... i thoguht I just verified i didn't do that and here we stand (literally the point of building z inside the loop). That alone appears to have resolved most of my issue by moving it out side of the Next i. Now, to look at the suggestions... I didn't know about mid$ so that is something else to read-up on. Thanks for the thoroughly explained response! I'll work to implement the more superior code. \$\endgroup\$ – Cyril May 31 '19 at 16:04
  • \$\begingroup\$ @Cyril FYI Rubberduck flags uses of Mid with UntypedFunctionUsage inspection results. \$\endgroup\$ – Mathieu Guindon May 31 '19 at 16:06
  • \$\begingroup\$ When I used to join you folks in chat as you were working through some early dev stuffs I attempted to download and use RubberDuck, but I had some... difficulties (management approval and authorization on the work comps). May try it again, as they have given me more admin rites and the sort due to some of my projects \$\endgroup\$ – Cyril May 31 '19 at 16:08
  • \$\begingroup\$ It doesn't require any admin rights to install anymore, and you can register it per-user now =) \$\endgroup\$ – Mathieu Guindon May 31 '19 at 16:08

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.