Code readability

[cristianbuse]

Making code compact could be a good practice in moderation. However, my opinion is that this repo does this in detriment of readability and maintainability. My hope is that the below will bring solid arguments to back my opinion and possibly determine changes to this repositorya and style of coding.

I will address two important points:

• the use of : to put multiple statements in one line
• declaring multiple variables on the same line

in terms of how readability, maintainability and debugging is affected.

---

I also use the : to put multiple statements in one line, like in the following scenario. Consider the code:

Dim lowRow As Long
Dim uppRow As Long
Dim i As Long

lowRow = LBound(arr, 1)
uppRow = UBound(arr, 1)
i = 1

which is the most basic way that most coders will write code. My preference would be to rewrite it like this:

Dim lowRow As Long: lowRow = LBound(arr, 1)
Dim uppRow As Long: uppRow = UBound(arr, 1)
Dim i As Long:      i = 1

for several reasons:

• visually, it mimics how other languages allow to declare a variable while also assigning a starting value. Of course, VBA defaults the variables based on the data type but this form of writing can be a visual aid
• it does not interfere with debugging because on each line of code, there is a single statement that can have a breakpoint and that is the assignment following :. Dim statements cannot have breakpoints anyway
• some space is saved
• readability is not affected and it can be argued that it is actually improved as the idea of assigining a default value happens near the declaration

It is generally an accepted best practice, across most languages, that declaring each variable on it's own separate line is more human readable. Here's why:

• the human eye is best accustomed to have text aligned to the left and numbers to the right
• the risk of unexpected behaviours is reduced e.g. when variables are added, renamed, removed
• it's easier to add a comment for each variable, if needed, even at a later stage. Note that a good variable name removes the need for a comment in most cases
• changes are much more apparent when version control is used. Instead of mid-line changes it's cleaner to have single-line additions/changes/deletions. Also, branch merging is easier

Even more so, VBA is not very friendly when it comes to declaring multiple variables on the same line. Most users don't even realize that each variable's type must be declared explicitly and something like Dim a, b As String actually declares a as Variant. This could be an issue if a different user is changing code at a later stage and starts deleting types because he/she considers that the line should be shorter.

Let's consider an actual example from this repo:

Public Function StringToHex(ByVal str As String) As String
    Dim i As Long, b() As Byte, hexStr As String
    b = str: hexStr = "0x" & Space(Len(str) * 4 + 2)
    For i = 1 To UBound(b) + 1
        Mid(hexStr, i * 2 + 1, 2) = Right$("0" & Hex$(b(i - 1)), 2)
    Next i
    StringToHex = hexStr
End Function

Let's rewrite it in the basic way, without any : statement delimiters:

Public Function StringToHex(ByVal str As String) As String
    Dim i As Long
    Dim b() As Byte
    Dim hexStr As String
    
    b = str
    hexStr = "0x" & Space(Len(str) * 4 + 2)
    For i = 1 To UBound(b) + 1
        Mid(hexStr, i * 2 + 1, 2) = Right$("0" & Hex$(b(i - 1)), 2)
    Next i
    StringToHex = hexStr
End Function

or more compact by using : statement delimiters:

Public Function StringToHex(ByVal str As String) As String
    Dim i As Long
    Dim b() As Byte:      b = str
    Dim hexStr As String: hexStr = "0x" & Space(Len(str) * 4 + 2)
    
    For i = 1 To UBound(b) + 1
        Mid(hexStr, i * 2 + 1, 2) = Right$("0" & Hex$(b(i - 1)), 2)
    Next i
    StringToHex = hexStr
End Function

I think both the 2nd and the 3rd form of the function are superior to the 1st when it comes to readability, future maintenance, debugging and source control.

---

The above example might not be too obvious. Let's dicuss lines 286 to 314 from this gist.

Although I know this last gist example was intended to be written in a single function for portability, I trully believe that end users should make the effort to copy-paste multiple related functions. It almost feels that the end users are indirectly abusing the good will of the author, without knowing. Since copy-paste is involved anyway, it might as well include multiple adjacent methods. So, there's no real reason to have such a long function. On the contrary, the approach forces a very complex method with tens of variables which in turn forces saving a few lines of code space by declaring too many variables on the same line and using : to put multiple statements on the same line. This leads to tens of minutes of more work in trying to debug an issue (or to add functionality) when if fact the final users might only spend 5 seconds extra to make sure they copied all the needed methods which are anyway adjacent.

Dim cid As String, fileNum As Long, line As Variant, parts() As String
Dim tag As String, mainMount As String, relPath As String, email As String
Dim b() As Byte, n As Long, i As Long, size As Long, libNr As String
Dim parentID As String, folderID As String, folderName As String
Dim folderIdPattern As String, fileName As String, folderType As String
Dim siteID As String, libID As String, webID As String, lnkID As String
Dim syncID As String, mainSyncID As String
Dim syncFind As String, mainSyncFind As String
Dim odFolders As Object, cliPolColl As Object, libNrToWebColl As Object
Dim sig1 As String: sig1 = MidB$(Chr$(&H2), 1, 1)
Dim sig2 As String: sig2 = ChrW$(&H1) & String(3, vbNullChar) 'x01 (x00 * 7)
Dim vbNullByte As String: vbNullByte = MidB$(vbNullChar, 1, 1) 'x00
Dim buffSize As Long, lastChunkEndPos As Long, lenDatFile As Long
Dim lastFileUpdate As Date, coll As Collection, duplicateCheck As Collection
Dim dirName As Variant, wDir As Variant, settDirIsDuplicate As Collection
#If Mac Then 'Variables for manual decoding of UTF-8, UTF-32 and ANSI
    Dim j As Long, k As Long, m As Long, ansi() As Byte, sAnsi As String
    Dim utf16() As Byte, sUtf16 As String, utf32() As Byte
    Dim utf8() As Byte, sUtf8 As String, numBytesOfCodePoint As Long
    Dim codePoint As Long, lowSurrogate As Long, highSurrogate As Long
    ReDim b(0 To 3): b(0) = &HAB&: b(1) = &HAB&: b(2) = &HAB&: b(3) = &HAB&
    Dim sig3 As String: sig3 = b: sig3 = vbNullChar & vbNullChar & sig3
#Else 'Windows
    ReDim b(0 To 1): b(0) = &HAB&: b(1) = &HAB&
    Dim sig3 As String: sig3 = b: sig3 = vbNullChar & sig3
#End If

Dim settPaths As Collection: Set settPaths = New Collection
Dim settPath As Variant, clpPath As String

To make things worse, variables are declared nowhere near where they are actually used which adds to the mental strain of trying to keep the whole function in mind when performing maintenance. Since the number one priority of any programmer should be to manage complexity, compact code like the one above should be avoided.

---

Control structures should not use the : statement delimiter.
There is no real gain in having this:

If utf16le(j + 1) = 0 Then
    ansi(i) = utf16le(j): j = j + 2
Else: ansi(i) = &H3F: j = j + 2 '&H3F = "?"
End If

as opposed to this:

If utf16le(j + 1) = 0 Then
    ansi(i) = utf16le(j)
    j = j + 2
Else
    ansi(i) = &H3F '&H3F = "?"
    j = j + 2
End If

The space save does not justify the loss of readability.

It can be argued that this:

For i = 1 To UBound(ansi): ansi(i) = Asc(Mid(utf16leStr, i, 1)): Next i

saves space and is not too long to cause any issues.
However:

For i = 1 To UBound(ansi)
    ansi(i) = Asc(Mid(utf16leStr, i, 1))
Next i

is more readable, generally expected and it allows breakpoints on all 3 lines if needed.

[guwidoe]

You are right about everything, thanks for making the effort to write this. I should really grow out of my time of having fun with the syntax 😁

Since I'm no longer working on all this alone and people like you actually read my code I think it would be disrespectful not to follow your advice which I understand is the generally accepted standard (for a reason).

Also, because the current GetLocalPath function doesn't fit into the StackOverflow answer anyways, I don't have that excuse anymore.

Will change stuff as soon as I get to it!

[guwidoe]

Addressed most of the things you brought up here: 0aae034

If you have additional requests please let me know, otherwise, I'll close the discussion for now :)