: If you have any thoughts on this.
The code you have provided will lead you down the road to
spaghetti code. As a programmer, much of your time is spent
maintaining old code, not writing new code. Worse, you will be
maintaining someone else's old code.
Don't assume that the code you are writing today will be your
own code in the future. You will be a different programmer in six
months and you will not understand why or how you wrote what you
wrote. There is no better way to make code more maintainable for
your future self than to use good programming practices today and
to write it with that other programmer in mind.
This code doesn't not accomplish anything unless you are using
EndOfData and Unique_Count outside the sub. If you are, then you
need a function, not a sub.
If you need a result, learn how to write functions. Whether
they are user defined functions in sheets or functions in
programs, never use a VBA sub in place of a VBA function.
Turn on Option Explicit and declare all your variables.
Declare them in the smallest procedure possible. It is doubtful
you need EndOfData or vCell outside the sub. Declare it inside the
sub. It will not be available outside the sub or function.
: Sub Unique_Managers()
: Dim unique As Object
The norm is to indent the code inside a procedure, not to
indent the first line of a procedure. A two or four space tab is
most common. This is a standard programming practice. Write it
like this. Keep the same indent until you get to the End Sub
statement.
Sub Unique_Managers()
Dim Unique As Object
Programs are written for other humans to read. If you use an
odd convention, other programmers are going to wonder why. For
example, why is the second line not indented and the first line
indented two spaces?
This is not consistent indentation.
: Set Unique = CreateObject("Scripting.Dictionary")
: EndOfData = Cells(Rows.Count, 1).End(xlUp).Row
This is consistent indentation.
Set Unique = CreateObject("Scripting.Dictionary")
EndOfData = Cells(Rows.Count, 1).End(xlUp).Row
Each group of lines in a program is called paragraph. Related
lines tend to fall in the same paragraph. What is the relationship
between Unique and EndOfData? There is no right or wrong answer.
If you see a relationship, there is a relationship. If you do not
see a relationship, add white space between the lines.
I hate that you choose to indent the comment two spaces in
each paragraph, but you are consistent.
: ' Populate dictionary
: For Each vCell In Range(Cells(3, 1), Cells(EndOfData, 1))
: If Not Unique.exists(vCell.Value) Then
: Unique.Add vCell.Value, vCell.Value
: End If
: Next vCell
: ' Count unique items
: Unique_Count = Unique.Count
:
: ' Clean up
: Set Unique = Nothing
: End Sub
But, you have indented some blocks of code and not others. To
maintain consistency, the code should look like this. Yes, that
extra four space indent is important. Even in an email message. It
screams, "I am competent."
' Populate dictionary
For Each vCell In Range(Cells(3, 1), Cells(EndOfData, 1))
If Not Unique.exists(vCell.Value) Then
Unique.Add vCell.Value, vCell.Value
End If
Next vCell
' Count unique items
Unique_Count = Unique.Count
' Clean up
Set Unique = Nothing
End Sub
Spaghetti code leads to errors which are almost impossible to
debug. It is called spaghetti code because tracing the value of
variables which are not properly passed into and out of other
procedures is like finding the other end of a piece of pasta on a
plate. You would be amazed how many tiny little programs become
million line behemoths full of code no one can debug.
It looks like you are using Unique_Count in another part of
your program. If that is the case and if that is all you need then
this procedure should be a function, not a sub.
You will not become a better VBA programmer until you can
write functions. If my guess is correct, change this back to a
function. Don't give up and make it a sub. You are robbing
yourself and your organization by doing that.
This is not consistent indentation.
: Sub Unique_Managers()
: Dim Unique As Object
:
: Set Unique = CreateObject("Scripting.Dictionary")
: EndOfData = Cells(Rows.Count, 1).End(xlUp).Row
:
: ' Populate dictionary
: For Each vCell In Range(Cells(3, 1), Cells(EndOfData, 1))
: If Not Unique.exists(vCell.Value) Then
: Unique.Add vCell.Value, vCell.Value
: End If
: Next vCell
:
: ' Count unique items
: Unique_Count = Unique.Count
:
: ' Clean up
: Set Unique = Nothing
: End Sub
This is consistent indentation.
Sub Unique_Managers()
Dim Unique As Object
Set Unique = CreateObject("Scripting.Dictionary")
EndOfData = Cells(Rows.Count, 1).End(xlUp).Row
' Populate dictionary
For Each vCell In Range(Cells(3, 1), Cells(EndOfData, 1))
If Not Unique.exists(vCell.Value) Then
Unique.Add vCell.Value, vCell.Value
End If
Next vCell
' Count unique items
Unique_Count = Unique.Count
' Clean up
Set Unique = Nothing
End Sub
Gee, I sure hope EndOfData and vCell are not used somewhere
else in the script for some other reason. If one is, this sub just
clobbered that value. Hope it wasn't important!
This code operates on a specific range in a spreadsheet. It
can never be used to find the unique number of managers on another
sheet and the manager list can never be moved. It will break if
Excel it is used properly. That is not a good thing.
EndOfData = Cells(Rows.Count, 1).End(xlUp).Row
' Populate dictionary
For Each vCell In Range(Cells(3, 1), Cells(EndOfData, 1))
The function I wrote originally, will find the unique number
of items in any range, not just the managers range. It was
independent of the where the managers range was maintained. It
operated just as well on rows as on columns and accepted mutiple
rows and columns. There is nothing wrong with specialization, but
use it wisely.
One last note. Write sub and functions to do one thing and to
do only that one thing well. If you need to do two tasks, write
two procedures.