Commonmark migration

edited Jun 10, 2020 at 13:24

Community Bot

• 1

Inline Side-by-side Side-by-side Markdown

answered Jun 17, 2015 at 2:23

Mathieu Guindon

• 75.5k
• 18
• 194
• 467

#Naming.

Did you notice, every single identifier in the VBA standard library (or any other library you might reference, for that matter) uses PascalCase? Following standard conventions make your code easier to read, and therefore to maintain.

Keep camelCase for locals and parameters. YELLCASE or UPPER_SNAKE_CASE is a commonly used convention for constants, so I wouldn't change that.

I'm not sure what casing this is using:

Sub FormatasDatex()

CamelCase would be FormatAsDateX, but that "X" only makes me want to dig into the implementation to see what it stands for... and it turns out it just seems an arbitrary suffix that tells me nothing about the date format - actually, the date format is much less relevant than the fact that the procedure is an extremely specialized piece of code that formats some very specific cells on a very specific worksheet.

That's very, very frail: if a worksheet is ever inserted anywhere before the 3rd sheet, you have a bug.

I need to say something about this prefixing pattern:

Dim aCell As Range, bCell As Range

Set wsIndex = aIndex
Set wsImport = bImport
Set wsFinal = cFinal

aCell and bCell aren't any better than cell1 and cell2 (which are just as awfully meaningless) - if aCell is a search result, then why not call it searchResult? Prefixes a, b and c seem completely arbitrary - and if they denote a sequence of some sort, that sequence seems outside the scope of the procedure and as such, doesn't belong there.

---

###DoSomething

Procedures do something. Their names should always start with a verb that describes that. If the only name you can think of is vague and doesn't really describe what's going on, then chances are that you're looking at a procedure that's doing too many things. But I'll get back to that.

A procedure called xlSpeed doesn't say anything about what it's doing. Looking at its implementation and usage makes me wonder why it's not just called ToggleWaitMode with an Optional wait As Boolean = True parameter. Seeing that DisplayPageBreaks = False has no toggle state, makes me wonder... do page breaks really make that much of a difference when ScreenUpdating is disabled?

You know how property getters have this Property Get keyword in them, and return the property value? Any procedure that starts with Get, that doesn't return anything, has a horrible name. I'm completely confused with what GetRaw is supposed to be doing. My only clue is this comment:

Dim wsImport As Worksheet 'Raw

...which only makes sense after I've seen that GetRaw procedure and went WTF over it.

---

This is overly verbose:

If optimize Then
 If .Calculation <> xlCalculationManual Then .Calculation = xlCalculationManual
Else
 If .Calculation <> xlCalculationAutomatic Then .Calculation = xlCalculationAutomatic
End If

How about this?

.Calculation = IIf(optimize, xlCalculationManual, xlCalculationAutomatic)

Avoid double negatives in conditionals:

If .ScreenUpdating <> Not optimize Then .ScreenUpdating = Not optimize

"If screen updating is not not then". What's the cost of reassigning the same value, versus that of accessing it twice? Get rid of that confusing wording, and Keep It Simple, Stupid:

.ScreenUpdating = Not optimize

---

#Object Model Usage

The Microsoft Excel object model exposes a Worksheets and a Sheets collection - if your code can't deal with chart sheets, then stick to the Worksheets collection.

As far as best practices go, this isn't it. Your intention is to reference a very specific worksheet, with very specific content. Referring to worksheets by their index makes the whole thing fall apart, the minute a user decides he prefers having the worksheets sorted by name, and starts rearranging the order of things.

All worksheets have a Name property, but the user can change that as well. The single safest way to refer to a specific worksheet, is using its CodeName. And best of all, that code name happens to be a workbook-scoped object reference to the Worksheet object you're after.

If I understand correctly, you have a global-scope reference to ThisWorkbook.Worksheets("Final") with cFinal, that you copy to wsFinal, ...when the worksheet in question might have code name Sheet23. Why not give the special sheet code name FinalSheet, and ditch all the global variables and their copies?

###Consistency.

What's the difference between this:

Set rngs = ThisWorkbook.Sheets(2).Cells

And that?

wsFinal.Range("D2:D" & lastRow).Value = Sheets(1).Range("H2").Value

Nevermind that you should be using the Worksheets collection here - my question is, why is it fully qualified in one place, and implicitly qualified in the other?

---

#Parameterize.

Take this short little procedure as an example:

Sub transpose2()
 Sheets(2).Range("A1").EntireRow.Copy
 Sheets(1).Range("A2").PasteSpecial Transpose:=True
 Range("A1").ClearOutline
End Sub

Ignoring the fact that it's implicitly Public (why is it even in another module? who else is calling it?) and its poor name (what happened to transpose1?), ...and that Range("A1") is implicitly referring to the active sheet (danger!), ...what was I going to say here?

Ah, yes. Parameters. Consider:

Public Sub CopyPasteSpecialTranspose(ByVal source As Range, ByVal destination As Range)
 source.Copy
 destination.PasteSpecial Transpose:=True
 Application.CutCopyMode = False
End Sub

This procedure says what it does, and does what it says. Nothing more. Clearing outlines on the active sheet is a rather surprising side-effect of transposing a range... especially if it just so happens that the active sheet isn't one of the worksheets involved in the copy+paste operation.

By passing in source and destination parameters, you've now written reusable code.

---

#S.R.P.

Single. Responsibility. Principle. Do one thing, do it well. MainProcess is way too long to be doing that. Extract methods, my friend. Break that monster down into pieces that fit a screen, and have a name that says exactly what's going on.

Pass your "dependencies" as parameters, avoid implicit references to active sheet (or to anything for that matter - be explicit!), and regroup methods logically - don't send a handful of procedures into another module just for the heck of it!

A code module should read and unfold like a story: at the top you have a high level of abstraction with abstract concepts (CopySourceData, FormatOutputSheet, etc.), followed by more and more specialized methods with a lower and lower level of abstraction, that culminate with the knowledge of all the specific range addresses and other implementation details: if you want to know what the code does, you read the top of the module; when you want to know how it does it, you read the bottom.

I'd recommend Robert C. Martin's Clean Code for more details about abstraction levels, and many, many other things. The code samples are Java, but the concepts are pretty much language-agnostic.