I would like to add a few more pieces of advice to what @TinMan has already provided.
Good naming is one of the most useful and at the same time hardest things in programming. When you have to come back to your procedure 6 months in the furture, you will thank yourself for using descriptive names. In this case, the procedure is relatively short so that understanding what it does is not too hard, but in more complicated code, good names can make a huge difference.
Because of this, my advice is to go beyond the advise @TinMan has provided and to generally use descriptive names. (It is no problem if they get longer in the process.) E.g. the name
rng does not really tell you anything about what it is; calling it
sourceRange would certainly convey its purpose better.
Single Responsibility Principle
Another good guiding principle is the so called single responsibility principle. In short, it says that a usint of code should always only be responsible for one and only one thing. This makes it a lot easier to understand and modify code.
Getting this right is rather hard. However, there is a one rough guidline that can help: whenever you have the urge to add a header comment, you probably want to extract a procedure or function for whatever is done in the section.
In your case, one responsibility is to know how to extract values from a range. You could define a function to do that as follows.
Private Function DistinctValues(inputRange As Excel.Range) As Collection
Dim allValues() As Variant
allValues = inputRange.Value
Dim uniqueValues As Collection
Set uniqueValues = New Collection
Dim currentValue As Variant
For Each currentValue In allValues
On Error Resume Next
uniqueValues.Add currentValue, currentValue
On Error Goto 0
Set DistinctValues = uniqueValues
If you ever want to change how to get distinct values, e.g. using a
Scripting.Dictionary, you just have to change it in this one place. Moreover, your code is easier to understand if instead of
var = Range("B2:B7")
For Each a In var
arr.Add a, a
Dim certificatesRange As Excel.Range
Set certificatesRange = Range("B2:B7")
Set distinctCertificates = DistinctValues(certificatesRange)
You could also separate the actual assembling of the new table into a procedure that takes some base point, e.g. the upper-left corner of the target range, a collection of column headers, one of row headers and the data source range.
With this, let us call it
BuildPivotTable, your top procedure would look something like this:
Public Sub PivotData
Dim sourceRange As Excel.Range
Set sourceRange = Range("A2:C7")
Dim columnHeadersRange As Excel.Range
Set columnHeadersRange = Range("B2:B7")
Dim distinctColumnHeaders As Collection
Set distinctColumnHeaders = DistinctValues(columnHeadersRange)
Dim rowHeadersRange As Excel.Range
Set rowHeadersRange = Range("A2:A7")
Dim distinctRowHeaders As Collection
Set distinctRowHeaders = DistinctValues(rowHeadersRange)
Dim targetRange As Excel.Range
Set targetRange = ActiveSheet.Range("E1")
BuildPivotTable targetRange, distinctColumnHeaders, distinctRowHeaders, sourceRange
This separates getting the data from doing things with it. Should you want to change where you get your data, you will no longer have to find the appropriate passages between the code doing things with the data. You can even further enhance this by extracting the passages getting the various ranges into their own functions responsible to know where to get the particular data from.
Again, this is not that cruicial for this size of method, but as things grow larger, which tends to happen rather fast whan adding functionality to things, a good separation of responsibilities can help a lot; it certainly justifies the extra code you have to write to achieve it.
Do not Reuse Variables For Different Things
In your code, you first use the variable
arr for distinct names, then for distinct certificates. This makes it harder to follow what the the collection alrady contains and actively hinders good nameing: you cannot name it after what its purpose is if it has multiple ones.
It really does not cost a lot to generate yet another object. So such micro-optimizations should be avoided in favor of ease of reading the code.
Declare Variables Close to Their First Usage
Somewhat related to the last point, it is usually better to declare variables as close to their first usage as possible. This has the advantage that you cannot accidentaly add an access to the variable before that point (The compiler will yell at you.) and that you can be sure at that point that you have a clean object. E.g. a collection will not already contain something.
I know that this contradicts VBA style guids. However, those have been written in the 90s and which practices are deemed useful has involved in the past two decades.
Note that this guideline is much less relevent in short methods following the single responsibility principle.
After the general pieces of advice applicable to basically all programming languages, let me come to some more Excel and VBA specific point.
@TinMan already pointed out to possible enhancemets to make the code work with dynamic ranges of data. I would like to add two options: names ranges and list objects.
I am sure you are aware that you can name ranges in Excel either by writing in the address field in the top left or using the names manager in the formulas tab. You can use these names to specify ranges. If you define a named range
A2:A7, you can get the range for your names via
Worksheets("Sheet1").Range("Names"), which makes you independent from the specific design of the source sheet.
Even better would be to turn the input range into an Excel table, which we will call
SourceTable. Then, you can access it as a
WorkSheets("Sheet1").ListObjects.Item("SourceTable"). Moreover, if you add another row, it will simply expand to also contain the new row. This is a lot more convenient than the approach with names ranges, which have to be adjusted to deal with new rows at the bottom.
You can Save the Sheet You Are Working With in a Variable
@TinMan already suggested using a
With block to hold the worksheet you are working with. However, that will not work well when you have to pass it to some method. Instead, you can simply
Set assigne the worksheet to a varable of type
Explicitly Calling Default Members
Default members are members on objects that get used automatically when the object is used in a
Let assignmant, i.e. an assignment without the
Set keyword. They are a source of a lot of surprising behaviour, and bugs. Thus, you should always prefer to call the curresponding member explicitly. For
Range, this means using
Cells(l + 1, 5) = arr(l) you actually call
Cells(l + 1, 5).Value = arr(l), provided
arr(l) contains a value type. If it contained a
Range. the call would translate to
Cells(l + 1, 5).Value = arr(l).Value.
Since there is not too much data in this example it is not rally a performance problem, but
Collections are not designed to be iterated using indized. You should use a
For Each loop instead. Unfortunately, it is not possible to use value types as the type for the item to pick from the loop. However, every object type and
Variant will work.
As New is Usually Not a Good Idea
You actually use the capabilitis of the
As New declaration
arr As New Collection, which is not seen to often. This declaration has the rather surprising effect of implicitly adding
If arr Is Nothing Then Set arr = new Collection in front of every access to
Although this can be used here to clear the variable by setting it to
Nothing, it would be much clearer to simply set it to
New Collection instead.
Because this behaviour is surprising to most poeaple, I would generally advise against using
As New declarations. Moreover, it hurts performance a bit because of the constant checks against
Declare the Type of All Variables
It is already good to see that you declared the type of nearly all variables. However, you did not explicitly declare
a as a
Variant. This lets is vanish against the other declarations all using an
As Type declaration.