5
\$\begingroup\$

I have a file with more than 100k rows, but the structure is simple:

Date      _|_Name-Position-Color_|_Summ
17.11.2015 |"Name1               | 8813,52
           | Position1           |
          _|_Color1"            _|_
19.08.2015 |"Name2               | 3587,86
           | Position3           |
          _|_Color5"            _|_
12.01.2015 |"Name3               | 14,63
           | Position16          |
          _|_Color7"            _|_
07.12.2015 |"Name4               | 7129,97
           | Position11          |
           | Color3"             |

Result should be twelve identically formed tables from "Jan" to "Dec" sliced from "Name-Position-Color" column sheet1 placed into sheet3 as "Name-slice" -column and "Position-slice" -head row. The "color" part is no longer needed. Tables should be filled with multiplied "Name-slice" by "Position-slice" including a period in which they're positioned in the first list. I hope this is informative enough to understand.

So, I've managed to write the macro below, but it works really slowly even when I have only 228 rows in the list. It had worked fast just before I've added a calculation part. I think object programming could save some time, but I haven't learned it yet. I would really appreciate if someone could tell me the way to improve my code, so it'll work faster. Any advice would be really helpful too.

Sub tablesByMonths()

'def column in sheet1
colNum1 = 2
'def column in sheet3
colNum3 = 2 '2 is minimal for correct macro work
'def last row in sheet1
lastRow1 = Worksheets("Sheet1").Cells(Rows.Count, colNum1).End(xlUp).Row
'def first row in sheet1
firstRow1 = Worksheets("Sheet1").Cells(Rows.Count, colNum1).End(xlUp).End(xlUp).Row + 1
'def last row in sheet3
step = 2

Application.ScreenUpdating = False                  'turns off dynamic     screen update
Application.Calculation = xlCalculationManual       'turns off automatic formulas

'clears all used range in a sheet3
Worksheets("Sheet3").UsedRange.Clear

'this counts months from Jan to Dec
For per = 1 To 12

'def last row in sheet3
lastRow3_1 = Worksheets("Sheet3").Cells(Rows.Count, colNum3).End(xlUp).Row
'puts current number from per loop and adds "/01/2015"
Worksheets("Sheet3").Cells(lastRow3_1 + step, colNum3 - 1).Value = per & "/01/2015"
'converts date into month format
Worksheets("Sheet3").Cells(lastRow3_1 + step, colNum3 - 1).NumberFormat = "mmmm"

'loop through the entire list in a sheet1 column colNum1
For x = firstRow1 To lastRow1

    'def current cell value
    curVal1 = Worksheets("Sheet1").Cells(x, colNum1)
    'def first space position in curVal1
    spacePos1 = InStr(1, curVal1, Chr(10), vbBinaryCompare)
    'def second space position in curVal1
    spacePos2 = InStr(spacePos1 + 1, curVal1, Chr(10), vbBinaryCompare)
    'def first word in curVal1 cell and place it into sheet3
    Worksheets("Sheet3").Cells(lastRow3_1 + step - 1 + x, colNum3) = Mid(curVal1, 1, spacePos1 - 1)
    'def second word in curVal1 cell and place it into sheet3
    Worksheets("Sheet3").Cells(lastRow3_1 + step - 2 + x, colNum3 + 1) = Mid(curVal1, spacePos1 + 1, spacePos2 - spacePos1 - 1)

Next x

'def last row in a new list sheet3
lastRow3 = Worksheets("Sheet3").Cells(Rows.Count, colNum3).End(xlUp).Row
'def last row in a new list sheet3
firstRow3 = Worksheets("Sheet3").Cells(Rows.Count, colNum3).End(xlUp).End(xlUp).Row

'del replicas from list with names and sort in ascend order in sheet3
With Worksheets("Sheet3").Range(Worksheets("Sheet3").Cells(firstRow3, colNum3), Worksheets("Sheet3").Cells(lastRow3, colNum3))

    .RemoveDuplicates Columns:=Array(1), Header:=xlNo
    .Sort key1:=Worksheets("Sheet3").Cells(firstRow3, colNum3), Header:=xlNo

End With

'del replicas from list with positions and sort in ascend order in sheet3
With Worksheets("Sheet3").Range(Worksheets("Sheet3").Cells(firstRow3 - 1, colNum3 + 1), Worksheets("Sheet3").Cells(lastRow3, colNum3 + 1))

    .RemoveDuplicates Columns:=Array(1), Header:=xlNo
    .Sort key1:=Worksheets("Sheet3").Cells(firstRow3 - 1, colNum3 + 1), Header:=xlNo

End With

'def new last cell for list of positions in sheet3
lastRow3_2 = Worksheets("Sheet3").Cells(Rows.Count, colNum3 + 1).End(xlUp).Row

'transpose sorted list of items into head row
Worksheets("Sheet3").Range(Worksheets("Sheet3").Cells(firstRow3 - 1, colNum3 + 1), Worksheets("Sheet3").Cells(firstRow3 - 1, lastRow3_2 - firstRow3 + colNum3 + 1)) = Worksheets("Sheet3").Application.Transpose(Worksheets("Sheet3").Range(Worksheets("Sheet3").Cells(firstRow3 - 1, colNum3 + 1), Worksheets("Sheet3").Cells(lastRow3_2, colNum3 + 1)))
Worksheets("Sheet3").Range(Worksheets("Sheet3").Cells(firstRow3, colNum3 + 1), Worksheets("Sheet3").Cells(lastRow3_2, colNum3 + 1)).Clear

'def last row in a new list sheet3 after deleting dublicates (need a method of calling a function to do it repeatedly)
lastRow3n = Worksheets("Sheet3").Cells(Rows.Count, colNum3).End(xlUp).Row
'loop through list of names
For namesList = firstRow3 To lastRow3n

    For headRow = colNum3 + 1 To lastRow3_2 - firstRow3 + colNum3 + 1

        'takes position name of the current position in the head row list
        currentValue = Worksheets("Sheet3").Cells(namesList, colNum3) & Chr(10) & Worksheets("Sheet3").Cells(firstRow3 - 1, headRow) & Chr(42)
        Worksheets("Sheet3").Cells(namesList, headRow).Value = "0.00"                'def starting value
        Worksheets("Sheet3").Cells(namesList, headRow).NumberFormat = "#,##0.00"     'establishes cell format
        'loop through list in the base table
        For firstList = firstRow1 To lastRow1

            listValue = Worksheets("Sheet1").Cells(firstList, colNum1).Value
            'checks if value in the first list equal to the current combined value
            If listValue Like currentValue Then

                Worksheets("Sheet3").Cells(namesList, headRow).Value = Worksheets("Sheet3").Cells(namesList, headRow).Value + Worksheets("Sheet1").Cells(firstList, colNum1 + 1).Value

            End If

        Next firstList

    Next headRow

Next namesList

Next per

Application.ScreenUpdating = True                   'turns on dynamic screen update
Application.Calculation = xlCalculationAutomatic    'turns on automatic formulas

End Sub
\$\endgroup\$
5
\$\begingroup\$

First some code style points:

  1. Declare your variables! Not only does it help prevent a slew of bugs from cropping up, it makes it a lot easier to read and understand what your code is doing. Looking at only the code that was posted, I have no idea if they are local or global scope or what data type they are. These are important readability clues, which leads to the second point:
  2. Comments should primarily explain the why, not the what, and only the how if it isn't obvious. Who and when are strictly optional, but I generally remember if I've written a piece of code and Excel has file modified dates. For example - if I can't figure out that Application.ScreenUpdating = False "turns off dynamic screen update", then I probably don't have any business mucking around in there anyway. Which leads to the next point:
  3. Give your variables meaningful names. When going through a Sub this long, I shouldn't have to scroll all the way to the top in order to determine that colNum3 is the default(?) column in Sheet3. Your code should strive to be self documenting. There's a reason I can figure out that Application.ScreenUpdating = False means "stop the screen from refreshing". If the Office team had decided that the property should be named .HazBlitting and take the enum arguments xlRly and xlOmg, then it wouldn't be so obvious. By the same token, if the loop For per = 1 To 12 were For monthNum = 1 To 12, then it would be obvious enough that the comment 'this counts months from Jan to Dec would be unnecessary.
  4. If you have a variable that you never assign a different value to, make it a constant. This way it's both clear that the value isn't going to change in your code, and just as important, prevents you from accidentally assigning another value to it (see point 1). In your case, colNum1, colNum3, and Step should all be declared as constants.
  5. Taking point 4 one step further, if you perform the same calculation with 2 constants repeatedly, the result of that calculation should also be a constant. For example, you compute the value colNum3 + 1 at 13 different places in your Sub and do it in a deeply nested loop. In fact, there are only 11 times that you don't add one to it. While the code with happily compute that 2 + 1 = 3 millions of times without complaint, keep in mind that everyone already knows that if colNum3 is always 2, then colNum3 + 1 will always be 3.
  6. Expanding on point five even more... if you have to perform the same calculation repeatedly, it should be in a variable. Consider the calculation lastRow3_1 + Step. lastRow3_1 only changes once every time through your outer loop and Step is a constant. But in the 4 times that you use lastRow3_1 you never use it without adding Step to it. If you're going to use the result of a calculation more than once, store it in a variable and use that.

  7. Speaking of the Step variable, you should avoid using VBA keywords as variable names. Is this a typo? For Foo = 1 To Step? Sure looks like one until I realize that Step is being used as a variable. It also avoids code lines such as For Polka = Pause To Step Step Step, which makes my brain bleed out my ear a little bit. Luckily the parser doesn't let you do If Like Like Like Then or If Then And If Then, but I digress...

  8. Be consistent with your indentation and go in one level each Loop, If, Sub/Function, Select, Case, (and the others that don't immediately spring to mind because I do it so habitually now). I almost missed the fact that For per = 1 To 12 doesn't continue until the very bottom of the Sub. Indenting makes it obvious. That said, good job putting the loop counters after the Next at the end. That makes a huge difference (but doesn't mean you shouldn't indent too).
  9. _ is VBA's line continuation operator. Use it if, for example, you have a line of code that is 346 characters long before you indent it. It's a lot harder to understand what code is doing if you can't see it. By the time I finished scrolling to the right, it was the only thing I could see anywhere in the code editor.
  10. Speaking of scrolling, side-scrolling isn't the only scrolling that makes it more difficult to determine at a glance what code is doing. Vertical scrolling does the same thing. The second thing I did after popping this code into the VBE (see point 6 for the first thing I did) was to delete all of the empty lines and comment only lines (see point 2) so I could see more of it at the same time. Close, but not quite, even after closing the Immediate Window that makes me feel naked if it isn't docked at the bottom of the VBE window. The best way to address this is by splitting your Sub into smaller pieces that each address a more tightly focused aspect of the problem. Extract functions until you have a set of small, manageable pieces of code.

Performance and Excel specific:

Every time you type a . between two tokens (except in a quoted literal or decimal), you dereference an Object. This isn't free. Consider the code Foo.Bar. The compiler, and (because VBA is using IDispatch or IUnknown interfaces) the executing assembly says something like "What is this Foo you're referring to?". Then it goes and finds it. Then is says something like "Can I make a 'Foo' do 'Bar'?". Then it does Bar. The With blocks help with this. They tell the compiler to assemble in a way that says "Until further notice, do what I'm telling you to do on the Foo.". The other way you can do that is to put the reference into a variable.

When you do something like Set TargetSheet = ActiveWorkbook.Worksheets("Sheet3"), you not only eliminate one dereferencing each time you use it, but just as importantly, you avoid the possibility that something changes the ActiveWorkbook. All the work that goes into determining which Workbook is active, and where the Worksheets collection is in memory, and retrieving the Worksheet with the Name "Sheet3" is done once. This can add up really fast in a loop - your code does it per * (lastRow1 + (lastRow3 * (lastRow3_2 - firstRow3 + colNum3 + 1) * lastRow1))) times.

OK, so much longer than I intended at this point - take a stab at the issues above and the other answers and see if that makes your performance more acceptable. If not, feel free to post a follow-up question.

\$\endgroup\$
  • 2
    \$\begingroup\$ Expanding on 1.: Option Explicit makes the compiler shout at you when you don't do it, so I'd highly recommend it \$\endgroup\$ – Vogel612 Apr 1 '16 at 11:42
  • \$\begingroup\$ I couldn’t fit my answer to your comment here (it says I have too much characters), so I’ve made a new comment below. \$\endgroup\$ – Alex K Apr 1 '16 at 13:29
4
\$\begingroup\$

I can tell you what you can do to make your code run between 60 and 100 times faster (and maybe much more depending on what you're doing)... but it will require something of a rewrite. In short: never access workbook unless you absolutely have to (including all it's child elements, worksheets, ranges, etc.) . Instead, read everything in as an array, and do you changes on that array. Once you are done, redrop the array into the worksheet.

Private Sub Example()
    Dim theArray as Variant
    theArray = ActiveWorkbook.Worksheets(1).UsedRange
    someOperationsOnTheArray theArray, maybeSomeOtherParameters
    ActiveWorkbook.Worksheets(2).Range("A1:Z100") = theArray
End Sub

In this example, I pickup the data from worksheet 1 and drop it into theArray, only to do some operations on my data, and afterwards drop the array into worksheet 2.

Notice that after I call .UsedRange, I don't access the worksheet again until I am totally done with the data.

Unfortunately with some things you are stuck (like .NumberFormat'ing ranges) with accessing the worksheet, but otherwise if you stay away, thing will speed up DRAMATICALLY.

\$\endgroup\$
  • \$\begingroup\$ Thank you a lot! I'll keep that in mind. Never thought accessing workbook "eats" computer resources, furthermore time. \$\endgroup\$ – Alex K Apr 5 '16 at 5:17
  • \$\begingroup\$ I would add to this: if you can avoid accessing Excel objects in the first place, by making a direct data connection to the worksheet, that would be even faster. If you want to perform advanced manipulations of the data (sorting, grouping, filtering) it's nearly trivial to express in SQL what would take many lines of code to do if manipulating an array; as well as being far more maintainable (e.g. this or this) \$\endgroup\$ – Zev Spitz Aug 15 '18 at 10:35
0
\$\begingroup\$

This is a great explanation of what's going on under the hood! It is the fourth code in my life, but I thought it is quite good.

  1. Until now, I didn't understand why I should declare any variables, course it seemed to work fine without it.
  2. Didn’t get it. So, I should not place any comments opposite obvious functions, but explain the reason I use any complex ones?
  3. Thank you on this point. Was meeting the same need of scrolling up to remind myself the meaning of some variables, but still was blind to evaluate in naming them.
  4. Thank you again. Don’t even know what to say here…
  5. Didn’t know it’s being calculated more than once. I thought computer, once calculated, saves it in memory and just calls to it. So, never cared of declaring that kind of variable as a constant.
  6. I see, it should be more like preparative part with all the variables in it, and less like coding part where those declared variables are called and it is easier for computer to calculate and also are more transparent for the future reading.
  7. It looks funny the way you’re describing this point, especially Polka point. Guilty, I am. Didn’t notice it is a VBA keyword.
  8. Got it. It is really new way of thinking, I’ll get it inhabited.
  9. I've also tried to find out the meanings of _ and now there's helpful hint you gave me. I do understand that long rows of code are annoying and inappropriate way of coding, but didn’t know how to shorten it.
  10. Just into the point. It is like object-oriented programming. I fully understand the beauty of the method, but don’t have an experience of calling functions yet. Maybe you could suggest an example I could learn from?

Resume:

You mean there’s a reference to point 5: I need to declare variables at once, even Worksheets should be considered as variables in the very beginning of a Sub. You know, even now (before doing anything) I feel it makes sense. If macro goes for checking and applying the path every time it loops through each cell in each months range, it must be taking too much effort for calculating process.

Many thanks you for your time!

I wish I had such support by my side =)

\$\endgroup\$
  • \$\begingroup\$ Re #2, the main take-away is that if it's obvious what a line of code does, you probably shouldn't comment it - the comment just adds noise. If you take the suggestions from #3 and use meaningful variable and function names, much, much more will be obvious from the code itself, which means less and less comments. This page is a good read on self documenting code. \$\endgroup\$ – Comintern Apr 1 '16 at 17:27

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.