We know clean code when we see it. Clean code is a pleasure to read and maintain. Clean code makes its purpose obvious, and is easily extended or modified. I cannot recommend Robert C. Martin’s Clean Code: A Handbook of Agile Software Craftsmanship enough – to me it was an eye opener. Code examples are in Java, but the principles are mostly language-agnostic – and the realization that the vast majority of it could also be applied to VBA changed how I saw VBA code, quite radically over time.
Sometimes deeply rooted, some habits we’ve been carrying since forever – things we never even thought for a split-second could be second-guessed, things VBA programmers do, stem from how code was written back in the 1990’s.
VBA is essentially stuck in 1998. Most of its commonly agreed-upon best practices are from another era, and while developers in every single other language moved on to more modern conventions, a lot of VBA folks are (sometimes firmly) holding on to coding practices that are pretty much universally considered harmful today: this has to be part of why so many programmers dread maintaining VBA code so much.
Is Rubberduck enforcing any of this?
Rubberduck will never force you to change your coding style. If we implemented an inspection inspired by any of these guidelines, it was to make it easy to identify the code that doesn’t adhere to them – every inspection in Rubberduck can be disabled through inspection settings. You own your code, you’re in charge. Rubberduck is just there to help take action if you want to, not to boss you around.
In no particular order:
If you haven’t read Joel Spolsky’s excellent Making Wrong Code Look Wrong yet, by all means take the time now – it’s a very, very good read, and throughout the years I must have read it a dozen times, and linked to it a hundred times.
Done and bookmarked? Ok. So now you know Hungarian Notation was never intended to encode data types into variable names, and that this practice only came into existence because of an unfortunate misunderstanding. The intent was to use short prefixes to identify the kind of variable (not its data type), in the context of the application. This is Apps Hungarian – as opposed to Systems Hungarian. The former is very useful and still relevant to this day, the latter is essentially useless clutter. No modern naming best-practices encourage this unfortunate prefixing scheme – it may be a hard one to unlearn, but it’s worth it. Plus it spares the world from this …gem:
Dim oRange As Range
v-for-variant, i-for-integer, l-for-long (what one has to be the worst), s-for-string (or worse, “str”), o-for-object… all are useless prefixes that serve no purpose, obscure a variable’s name, and that’s just when they’re correct: when they’re incorrect or inconsistent, they start getting distracting and bug-inducing, on top of just being mildly annoying …or amusing. What does
strString As String (the ultimate tautology!) tell you about the purpose of a parameter? That’s right, nothing at all.
In BASIC 2.0 on a Commodore-64 you had a whole 2 meaningful characters to name your variables. You could use more, but the first 2 had to be unique.
How awesome is it that things have changed! In VBA an identifier can be up to 255 characters long. Programming isn’t about writing code, at least 90% of it is about reading code. The handful of keystrokes you’re saving are turning into tenfold the amount of time wasted investigating the meaning of these cryptic variables.
Stop stripping the vowels from variable names for no reason: they’re essential to convey meaning (at least without needing to then clarify in a comment). The few spared keystrokes aren’t worth all the “fun” you’ll have re-reading that code in a year’s time.
Wall of Declarations
I was taught to begin all procedure scopes with the declarations for all the variables in that scope, supposedly to enhance readability. For years, it seemed like a good idea – until I had to debug a 700-liner legacy procedure that started with a literal wall of declarations… with half of them not used anywhere, and the whole thing taking up more than a whole screen’s height. In fact, every single time I answered (or commented on) a question on Stack Overflow and noticed a variable wasn’t used anywhere, there was a wall of declarations at the top of the procedure.
Declare variables where you’re using them. That way you’ll never need to wonder if a variable is used or not, and you’ll never waste considerable time constantly scrolling up & back down, then back up, then down, when debugging a large procedure.
Code that is easy to maintain, is code that is easy to modify, and thus easy to refactor. Having 10 lines of declarations at the top of a procedure scope isn’t working in that direction: as the code changes, the maintainer will be more inclined to keep the style that’s in place, i.e. to append to the list of declarations so as to keep all the declarations together… whereas if there’s no such list in the first place, starting one will look wrong.
Procedures should be responsible for a little as possible. One thing, ideally. Whenever there’s a comment that looks like this in the body of a procedure:
'==== reticulate splines ====
It’s a missed opportunity: the procedure wants that chunk of code extracted into its own
ReticulateSplines scope, taking in parameters for whatever local variables it’s using… and this ties back to the Wall of Declarations: if the variables are declared close to where they’re first used, then extracting that chunk of code and knowing what declarations to bring over to the new scope, becomes much easier… and accidentally leaving unused variables behind is in turn much harder to do now.
Banner comments literally scream “I’m doing to many things!” – don’t split procedures with banner comments. Write smaller procedures instead.
Everywhere you look, in every standard type library you can include in a VBA project, everything uses a standard PascalCase naming style. By adopting a consistent
PascalCase naming scheme, you make your code blend in seamlessly. But this isn’t just a personal preference thing:
Snake_Case cannot be consistently applied to any object-oriented code written in VBA, because you can’t have a method named
Do_Something on an interface. The compiler will simply refuse to consider
InterfaceName_Do_Something as valid: because you used
Snake_Case on a public member name, your code is now broken and can’t be compiled anymore. And if you drop the underscores just for interface methods, then you’re no longer using a consistent naming style, and that’s a problem when consistency is king (and it is!).