5
\$\begingroup\$

I am trying to improve some code I did not write, mostly to make it easier for other people to understand (I found it to be really weirdly written). I attempted to rewrite it, and it does basically the same thing, using roughly the same processes.

However, the old one takes about 2 minutes to run with some data I recorded. The other takes well over an hour to work on the same data. What on earth is going on?

The data format they work on is:

Sample Data

I should add: I would use dictionaries, as I think they would be fastest, except the number of "TextX" is basically random. Sometimes only Text1 appears. Sometimes it goes all the way to Text20, or more.

Old code:

sub DivideSheet()
Application.ScreenUpdating = False

Dim name As String
Dim point_name As String
Dim SheetCount, sheetNumber As Integer
Dim RowCount, RowStart As Long
Dim Exist, RowEmpty As Boolean

Sheets("RAW DATA").Columns("A:A").TextToColumns Destination:=Range("A1"), DataType:=xlFixedWidth, _
    FieldInfo:=Array(Array(0, 1), Array(12, 1), Array(13, 1), Array(22, 1), Array(23, 1), Array(27, 1), _
    Array(31, 1), Array(36, 1)), TrailingMinusNumbers:=True

Exist = False
RowCount = 1
RowStart = 1
SheetCount = 1
point_name = Worksheets(1).Cells(1, 3)

For RowCount = 1 To 1048576                                       'loop through all the rows in the sheet

    If CStr(Sheets(1).Cells(RowCount, 3)) = "" Then             'If the cell isnt a new transmition
                                                                'do nothing
    ElseIf CStr(Sheets(1).Cells(RowCount, 3)) = point_name Then   'If the new transmition is from the same node
                                                                'Do nothing
    Else                                                        'If its a new node

        For SheetCount = 1 To Sheets.Count                      'loop through sheets

            If Worksheets(SheetCount).name = point_name Then      'If the sheet name matches point_name
                Exist = True                                    'set flag to true
                sheetNumber = SheetCount                        'Record Sheet number
                Exit For                                        'Exit the for loop
            End If

        Next SheetCount

        If Exist = False Then                                   'If the Node didnt have a sheet
            sheetNumber = SheetCount
            Worksheets.Add After:=Worksheets(SheetCount - 1)    'Create a sheet
            Worksheets(Sheets.Count).name = point_name            'Name it for the RTU
        End If

        Call CopyLine(sheetNumber, RowStart, RowCount - 1)      'Call the Copying function to copy the chunk of data

        RowStart = RowCount                                     'Set a new start point for the next chunk of data

        point_name = Worksheets(1).Cells(RowCount, 3)             'Set the Node the chunk of data will belong to

        SheetCount = 1                                          'Reset Variable
        Exist = False                                           'Reset Variable


    End If

Next RowCount

SheetCount = 1                                                  'Resets the SheetCount Variable


For SheetCount = 1 To Sheets.Count                              'Loops through Sheets

    Sheets(SheetCount).Columns("H:H").EntireColumn.AutoFit      'Autofits the H column

Next SheetCount

End Sub

Public Sub CopyLine(sheetNumber As Integer, RowStart As Long, rowNumber As Long)

    If Sheets(sheetNumber).Range("A1").Value = "" Then           'If its the first data to enter the sheet
                                                                    'Copy the chunk of data
        Sheets(1).Range("a" & RowStart, "h" & rowNumber).Copy _
        Destination:=Sheets(sheetNumber).Range("A1", Cells(rowNumber - RowStart + 1, "H"))

    ElseIf Sheets(sheetNumber).Range("H2").Value = "" Then       'If its the Second data to enter the sheet
                                                                    'Copy the chunk of data
        Sheets(1).Range("a" & RowStart, "h" & rowNumber).Copy _
        Destination:=Sheets(sheetNumber).Range("A2", Cells(rowNumber - RowStart + 2, "H"))

    Else                                                            'otherwise
                                                                    'Copy the chunk of data
       Sheets(1).Range("a" & RowStart, "h" & rowNumber).Copy _
       Destination:=Sheets(sheetNumber).Range("h1048576").End(xlUp).Offset(1, -7)

    End If
End Sub

My code:

sub DivideSheet()
Application.DisplayAlerts = False
Application.ScreenUpdating = False

Dim Point As String

Set wb1 = ThisWorkbook

wb1.Sheets("RAW DATA").Columns("A:A").TextToColumns Destination:=Range("A1"), DataType:=xlFixedWidth, _
    FieldInfo:=Array(Array(0, 1), Array(12, 1), Array(13, 1), Array(22, 1), Array(23, 1), Array(27, 1), _
    Array(31, 1), Array(36, 1)), TrailingMinusNumbers:=True

'loop through all the rows in the sheet
For i = 1 To wb1.Sheets(1).UsedRange.Rows.Count

    Point = wb1.Sheets(1).Cells(i, 3)
    If wb1.Sheets(1).Cells(i, 3) <> "" Then
        a = sheetexists(Point)
            If a Then
                Call CopyLine(a, i, Point)
            Else
                wb1.Sheets.Add after:=wb1.Sheets(Sheets.Count)
                wb1.Sheets(Sheets.Count).name = Point
                Call CopyLine(wb1.Sheets.Count, i, Point)
                wb1.Sheets(Sheets.Count).Rows(1).Delete
            End If
    End If
Next i

For Each wsh In wb1.Sheets              'Loops through Sheets
    wsh.Columns.AutoFit         'Autofits everything
Next wsh

End Sub

Public Sub CopyLine(a, i, Point)

Set wb1 = ThisWorkbook
j = i + 1
While (wb1.Sheets(1).Cells(j, 3) = "" Or wb1.Sheets(1).Cells(j, 3) = Point) And j <= wb1.Sheets(1).UsedRange.Rows.Count 'WorksheetFunction.CountA(wb1.Sheets(1).Rows(i)) = 0
    j = j + 1
Wend

wb1.Sheets(1).Range(wb1.Sheets(1).Cells(i, 1), wb1.Sheets(1).Cells(j - 1, 8)).Copy Destination:=wb1.Sheets(a).Cells(wb1.Sheets(a).UsedRange.Rows.Count, 1).Offset(1, 0)
i = j - 1
End Sub

Function sheetexists(ByVal Point As String) As Long
Dim a As Integer
Set wb1 = ThisWorkbook

sheetexists = 0
For a = 1 To wb1.Sheets.Count
    If wb1.Sheets(a).name = Point Then
        sheetexists = a
        Exit Function
    End If
Next a

End Function

I'm not crazy, right? Is there no fundamental difference between these two, other than mine being fewer lines, less circuitous, and generally not being wasteful? So why is my new code taking so much longer?

\$\endgroup\$
4
\$\begingroup\$

There are a couple places in your code where you've "de-optimized". I'm not sure which of these adds up to a hour of run-time, but these are the most egregious:


Removing variable declarations makes them implicitly Variant. The following are undeclared in DivideSheet(): wb1, i, a, and wsh. This is a huge performance penalty, because every time you use them, the underlying values need to be coerced out of the Variant. You also can't store strongly typed objects in a Variant, so that means every single one of your calls to wb1 and wsh is late-bound instead of early-bound (that's why you don't get any Intellisense when you type wb1. or wsh.. That's a ton of needless overhead, especially when you're in a loop. If you can early-bind, do it. Put Option Explicit at the top of the module, and then declare everything. Put the strong typing back in.

Note that the original code had this wrong too - lines like...

Dim RowCount, RowStart As Long

...only type the last variable as Long - the rest are Variant.


You also removed the strong typing from CopyLine. In the old code, it's declared as:

Public Sub CopyLine(sheetNumber As Integer, RowStart As Long, rowNumber As Long)

You have it as Public Sub CopyLine(a, i, Point), which means that all of the parameters are implicitly Variant. Granted, you're basically using Variant for everything, but again, you just added at least 3 extra unboxings for every call. Not to mention the extra work that Excel needs to do when you pass them as indexers to Cells. Put the strong typing back in.


You created implicit casts when you extracted the sheetexists function. You have it declared as returning a Long, but you declare a as Integer and then return that with sheetexists = a. Then when you call it here...

a = sheetexists(Point)

...you coerce it into a Variant (a is undeclared in DivideSheet), then on the next line you treat is as a Boolean...

If a Then

...before passing it (as a Variant) to CopyLine, where it gets used as a Variant of subtype Integer again: Sheets(a). (Whew!). Put the... wait for it... strong typing back in.


You forgot wb1 when you extracted the sheetexists function. You have Set wb1 = ThisWorkbook at the top of DivideSheet...and the repeat that call at the top of both sheetexists and CopyLine. If you want to make the 2 functions a bit more reusable, give them a Workbook parameter and pass that in. If you don't care, skip the variable declaration all together and just use ThisWorkbook explicitly. Note that this isn't the same as ActiveWorkbook - it's a hard reference to the ThisWorkbook class in the project the code is in.


The original code cached the result of the call to Sheets.Count. This code only calls Sheets.Count once when it set up the loop:

    For SheetCount = 1 To Sheets.Count                      'loop through sheets

        If Worksheets(SheetCount).name = point_name Then      'If the sheet name matches point_name
            Exist = True                                    'set flag to true
            sheetNumber = SheetCount                        'Record Sheet number
            Exit For                                        'Exit the for loop
        End If

    Next SheetCount

    If Exist = False Then                                   'If the Node didnt have a sheet
        sheetNumber = SheetCount
        Worksheets.Add after:=Worksheets(SheetCount - 1)    'Create a sheet
        Worksheets(Sheets.Count).name = point_name            'Name it for the RTU
    End If

Your code calls it 4 times every time you add a sheet. You actually don't even need it in the loop at all, because if you don't end up with Sheets.Count + 1 worksheets after you call Sheets.Add, then there is something very seriously wrong (this is a problem in the original code too). Cache values that you need to reuse.


Getting back to CopyLine, your code isn't equivalent at all. The original function is actually pretty efficient at what it does - it calculates what it needs to copy, then does that in a single operation. Your code doesn't perform a calculation at all - it counts row by row with this code (line continuations added for clarity):

While (wb1.Sheets(1).Cells(j, 3) = "" Or _
       wb1.Sheets(1).Cells(j, 3) = Point) And _
       j <= wb1.Sheets(1).UsedRange.Rows.Count
    j = j + 1
Wend

VBA doesn't short circuit the calls, so first you get the value of Cells(j, 3) (implicitly) twice, compare it to two different things, and (then this is the huge one) get the exact same value for wb1.Sheets(1).UsedRange.Rows.Count every time through the loop. Again, cache values that you need to reuse. This is much easier if you use a Do loop (there isn't an Exit While statement):

Dim rowCount As Long
rowCount = wb1.Sheets(1).UsedRange.Rows.Count
Do While j <= rowCount
    Dim cellValue As Variant
    cellValue = wb1.Sheets(1).Cells(j, 3)
    If cellValue = vbNullString Or cellValue = Point Then
        Exit Do
    End If
    j = j + 1
Loop
\$\endgroup\$
  • \$\begingroup\$ Huh. So I thought I was making it easier to read, and little did I know - I destroyed it! I had no idea using a variant was such a big deal, or making it recalculate everything would slow it down so much. Also, for CopyLine, I tried to make it so that it would basically split up the two countings - it counts to the new Point in sub DivideSheet and to where it has to copy in sub CopyLine, and updates it's record as it goes - I thought it would be easier for someone later to look at this and understand on first glance what was going on. [1] \$\endgroup\$ – Graham Perry Jan 31 '17 at 21:38
  • \$\begingroup\$ I will make these fixes and see how it goes. And thank you! [2] \$\endgroup\$ – Graham Perry Jan 31 '17 at 21:39
  • \$\begingroup\$ @GrahamPerry - To be honest, even without the comments, I thought the original code was much easier to understand what it was doing. I'd probably fix the performance issues with the original and leave it at that. \$\endgroup\$ – Comintern Jan 31 '17 at 21:41
  • \$\begingroup\$ If I may be so bold as to ask another question: I don't understand how CopyLine is so different between the two functions, really. The original code loops through every line, and I loop through every line - I just do half the counting in DivideSheet and half of it in CopyLine (so I guess my specific function is quite different but the net effect I thought was identical). The logic is identical - if you see nothing or Point, carry on, otherwise, copy. Or at least that's what I thought. What am I missing? \$\endgroup\$ – Graham Perry Feb 1 '17 at 16:51
  • \$\begingroup\$ @GrahamPerry - It's a nested loop. The original code finds the range to copy by calculating rowNumber - RowStart. The new code usies a While loop. The outer loop (in DivideSheet) calls CopyLine, which runs an inner loop. The original code doesn't loop inside CopyLine. \$\endgroup\$ – Comintern Feb 1 '17 at 16:55
3
\$\begingroup\$

Before changing anything functionality-wise, I usually like to layout things and improve the readability. The single, most impactful thing that could have been done, would have been to add an indentation level for every scope.

So instead of this:

Sub DoSomething(bar)
Dim foo
foo = 42
If bar > foo Then
    DoSomethingElse
End If
End Sub

You would have that:

Sub DoSomething(bar)
    Dim foo
    foo = 42
    If bar > foo Then
        DoSomethingElse
    End If
End Sub

That way it's much easier to tell where procedures start and end, especially when there's vertical whitespace between [groups of] statements (which is a very good thing!).

The next thing to do, to improve readability, is to refactor/rename the variables so that they have meaningful names. Now, doing that is theoretically innocuous - but in practice doing it without proper tooling (Ctrl+H?) can do real annoying damage, and doing it by hand is tedious and error-prone.

I, along with a number of other reviewers on this site (answerers on Stack Overflow, too), have developped Rubberduck (an ongoing, open-source VBE add-in project), specifically built to assist in these kinds of things (including indentation):

Rubberduck's refactor/rename tool

The next thing I'd look for, is for Rubberduck's inspection results. These things range from minor readability improvement suggestions to serious code quality issues.

You can run inspection results online by pasting your code in the box at the bottom of this page (it's much faster in the actual VBE though).

Here are a few:

  • Use of 'Call' statement

    The 'Call' statement is no longer required to call procedures, and only exists in the language to support legacy code that required it; it can be safely rewritten to an implicit call.

    So this:

    Call CopyLine(a, i, Point)
    

    Could be written like this:

     CopyLine a, i, Point
    
  • Implicit 'Variant' variables

    A variable whose type isn't explicitly declared, is implicitly 'Variant'. Consider making it an explicit 'Variant' if that's intended, or declare a more specific type.

    This applies to the parameters in the signature of the CopyLine procedure:

    Public Sub CopyLine(a, i, Point)
    

    Could be written like this:

     Public Sub CopyLine(ByVal a As Long, ByVal i As Long, ByVal Point As String)
    
  • Implicity public member

    Module members are public by default, which can be counter-intuitive. Consider specifying explicit access modifiers to avoid ambiguity.

    Procedure DivideSheet and function sheetexists are both implicitly Public, and it while CopyLine is explicitly Public, if it's only ever called from DivideSheet then it should be Private. Proper scoping/visibility ensures a cleaner API, especially with larger projects.

  • Implicit references to ActiveSheet/ActiveWorkbook

    Implicit references to the active sheet/workbook make the code frail and harder to debug. Consider making these references explicit when they're intended, and prefer working off object references.

    Whenever you use an unqualified Range or Sheets call, you're calling a member of the hidden _Global module, and your code implicitly works off the ActiveSheet. This affects the destination Range("A1") on line 9, and the Sheets.Count calls in lines 22, 23 and 25.

    Also worth noting, if you intend to work with Worksheet objects, then don't query the Sheets collection; use the Worksheets collection instead - Sheets can return with a Chart object, and a chart doesn't have a UsedRange member; that would blow up at run-time with error 438. Of course it doesn't "really" matter if your workbook doesn't have chart sheets, but the very moment it does, your code breaks. Best use the Worksheets collection.

  • Use meaningful names

    Identifier names should indicate what they're used for and should be readable; avoid disemvoweling, numeric suffixes, and 1-2 character names.

    Identifiers wb1, a, wsh, and arguably i and j could all use better, more descriptive names.

  • Empty string literal

    The built-in constant 'vbNullString' is a null string pointer taking up 0 bytes of memory, that unambiguously conveys the intent of an empty string.

    Instead of comparing to "", compare to vbNullString.

Now, these are nice, sometimes problematic, but somewhat superficial observations. The code has more serious issues:

  • 'Option Explicit' is not specified

    VBA will happily compile a typo: use 'Option Explicit' to prevent successfully compiling an erroneous program.

    I cannot stress this one enough. ALWAYS. USE. OPTION. EXPLICIT. Countless bugs and Stack Overflow questions could have been avoided, by simply typing these two words at the top of every module.

  • Undeclared variables

    Code that uses undeclared variables does not compile when Option Explicit is specified. Undeclared variables are always Variant, a data type that incurs unnecessary overhead and storage.

    Local variables wb1, i, a, and wsh aren't declared in DivideSheet; local variables wb1 and j aren't declared in CopyLine, and wb1 isn't declared in sheetexists.

I like that you've fixed these:

Dim SheetCount, sheetNumber As Integer
Dim RowCount, RowStart As Long
Dim Exist, RowEmpty As Boolean

Having multiple declarations in the same instructions sets you up for a common beginner trap: every single variable needs an explicit type. I'm pretty sure the author thinks they've declared both Exist and RowEmpty as Boolean variables (and SheetCount would be an Integer, and RowCount a Long), but SheetCount, RowCount and Exist are all implicit Variant, because their declaration is missing the As clause.


Just a quick note about casing & consistency: in VBA, public identifiers should be PascalCase, and locals/parameters should be camelCase. The code isn't consistent about it, and that can be distracting.

Another note, about comments: good comments say why, not what. Comments that merely state the obvious, or outright repeat verbatim what the code already says, are useless and should be removed.

Like this one:

'loop through all the rows in the sheet
For i = 1 To wb1.Sheets(1).UsedRange.Rows.Count

And these:

For Each wsh In wb1.Sheets          'Loops through Sheets
    wsh.Columns.AutoFit     'Autofits everything

DivideSheets is doing quite a number of things - I'd extract more methods out of it - but this is worrying:

Application.DisplayAlerts = False
Application.ScreenUpdating = False

It's never turned back on! Consider also switching Application.Calculation to XlCalculationManual, to avoid having Excel perform useless automatic calculations ...but don't forget to toggle it back to automatic when you're done!


I have to agree, there's something outrageous about taking a piece of code that completes in two minutes, and then seeing it take well over an hour to do the same thing after you've changed it a bit.

I'll leave the actual performance review to other reviewers... for now :)

\$\endgroup\$
  • \$\begingroup\$ Sorry, I should have mentioned that this is one part of a set of macros. wb1 is declared (publicly) in the code for a userform as workbook; likewise, Application.DisplayAlerts = True and Application.ScreenUpdating = True are present, just not in this particular module. Regardless, thank you. I made the adjustments that you suggested. \$\endgroup\$ – Graham Perry Jan 31 '17 at 20:58
  • \$\begingroup\$ If you're toggling on ScreenUpdating and DisplayAlerts in a different scope than where you're toggling it off, then it's definitely confusing. wb1 does not need to be global at all either. \$\endgroup\$ – Mathieu Guindon Jan 31 '17 at 21:01
  • 2
    \$\begingroup\$ @GrahamPerry just noticed... if wb1 is declared publicly in a form's code-behind, when you do Set wb1 = ThisWorkbook in DivideSheet you are NOT referring to the wb1 variable in the form's code-behind, because a form is an object, so you would have to qualify it with MyAwesomeForm.wb1 to access it. In other words, wb1 is an undeclared variable here... \$\endgroup\$ – Mathieu Guindon Jan 31 '17 at 21:24

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.