Raw exchange trade data analysis program

The following is a small snippet from a pretty large program written to analyze raw data and find specific trade structures. I am new to VBA and this is my first large scale program I have written in the language.

I definitely did things unconventionally and I wanted to ask for some review and pointers how to improve my code. I tried to do things I have done in other languages (at basic levels) and encountered some trouble so I did it in a way that achieved the goal. I'm sure you all agree when I say writing code that works is nice, but beautiful, succinct code is most important.

One thing specifically I had interest in doing was creating an object to manage the location of the program in the spreadsheet. I used the variable row_number to track the current row, but when I tried to set a range variable say Set r = Range("H" & row_number), row_number wouldn't update as it was incremented. Rather than having to write out all that code, writing just r would have been much more succinct, clean, and safe should I ever need to go back and make a change.

Also, the indenting might be a little off because I had to add spaces here and there so that it showed up as code rather than text, but I'm confident in my indenting practices.

```
'Deletes voided trades
Do
DoEvents

row_number = row_number + 1
tradeStatus = ActiveSheet.Range("M" & row_number)

If InStr(tradeStatus, "Void") >= 1 Then
ActiveSheet.Rows(row_number & ":" & row_number).Delete
row_number = row_number - 1

End If

Loop Until row_number = lastRow

'Resets row number in preparation for removing voided trades and finds the new last row
row_number = 2
lastRow = ThisWorkbook.Sheets("Trade_Data_Insert").Cells(Rows.Count, 1).End(xlUp).Row
colorImp = 23

'Highlights trade structures via time
Do
DoEvents

'Row color tester is a proxy for testing trade time "structure principle"
row_number = row_number + 1
row_color_tester = row_number
tradeTime = ActiveSheet.Range("B" & row_number).T

Things I like

-
Using Descriptive variable names. row_number, tradeTime, tradeMonth, legCounter. Now I don't have to keep guessing what your variables are supposed to be.

-
Good use of commenting. 'Row color tester is a proxy for testing trade time "structure principle" that took you 5 seconds to write and it saved me a good few minutes of trying to figure out what was going on and why.

-
Liberal, consistent use of Whitespace, indenting etc. It makes your code eay to read, easy to follow and easy to analyse. Keep it up.

Now, for your review:

Option explicit

Go to tools --> Options --> Require Variable declaration. This will automatically insert Option Explicit at the top of every code module from now on.

What does it do? It requires you to explicitly declare every variable like so:

Dim row_number as Long, legCounter as Long, lastrow as Long

Why is this important? Because if it is not enabled, then this happens:

lastrow = 2000

do while row_number = lastrows

lastrows is a typo, but VBA will assume it's an entirely new variable. Now you have a rogue variable in your program. Option Explicit. Always.

Naming

Proper Variable naming is one of the most important skills you can develop.

Names should be Clear, Concise and, above all, Unambiguous. Names should follow consistent conventions (and, preferably, those which are most widely-adopted). Essentially, I should be able to take any single line from your code, give it to some random person, and have them understand roughly what all the variables contain and what's being done to them.

This is good variable naming:

Loop Until row_number = lastRow

I took that completely out of context but I can tell exactly what's going on.

This is bad Variable naming:

If fLeg = sLeg And Range("J" & (row_number - 1)) = Range("J" & (row_number - 2)) Then
    mulLot = True
End If

fLeg, sLeg, mulLot what the hell are they? I haven't got a clue.

Variables should sound like what they are. Always.

Naming Conventions

Naming conventions are useful because they provide a common framework for people to 'parse' your code. In VBA, common conventions you should be aware of:

Procedure-level variables are written in camelCase (No spaces, all words capitalised except the first).

Dim finalRow as Long

Module and Global-level variables are written in PascalCase (No Spaces, all words capitalised).

Private ModuleVariable as variant
Public GlobalVariable as variant

Constants are written in SHOUTY_SNAKE_CASE

Public Const SOME_IMPORTANT_VALUE as string = "This Never Changes"

Function/Procedure Names are also written in PascalCase

Public Sub DoThisThing()

Event Procedures are written in Pascal_Snake_Case

Public Sub SomeObject_SomeEvent()

Refactoring

Refactoring is the process of splitting your code into small and smaller logical "Units". Often in the form of Subs/Functions.

Whenever you find yourself writing a comment to describe the next 10 lines of code E.G. "Deletes voided trades". Think ot yourself "Can I make this a separate Procedure?".

So, this:

'Deletes voided trades
  Do
  DoEvents

 row_number = row_number + 1
 tradeStatus = ActiveSheet.Range("M" & row_number)

 If InStr(tradeStatus, "Void") >= 1 Then
     ActiveSheet.Rows(row_number & ":" & row_number).Delete
     row_number = row_number - 1

 End If

Loop Until row_number = lastRow

Becomes this:

DeleteVoidedTrades wsTradeData, tradeStatusColNum, voidStringIdentifier

And then put this procedure somewhere:

Private Sub DeleteVoidedTrades(ByRef wsData As Worksheet, ByVal tradeStatusColNum As Long, ByVal voidStringIdentifier as String)

    Dim finalRow As Long
    finalRow = wsData.Cells(Rows.Count, tradeStatusColNum).End(xlUp).row
    
    Dim tradeStatus As String
    Dim row As Long
    
    For row = finalRow To 1 Step -1
        tradeStatus = wsData.Cells(row, tradeStatusColNum).Text
        
        If InStr(tradeStatus, voidStringIdentifier) >= 1 Then
            wsData.Rows(row).Delete
        End If
        
    Next row
        
End Sub

Deconstructing the above:

Clear Variable Names: DeleteVoidedTrades, wsData, finalRow, voidStringIdentifier, tradeStatusColNum.

Dynamic inputs: Determines finalRow. Allows you to specify the Void string. Allows you to specify the column to search.

Correct Loop mechanism: Do while should only be used when you might exit halfway through. For a fixed-length loop, use For. In this case, because deleting rows messes with row numbering, it goes from the final row down.

Deliberately specifies the worksheet: Relying on the active sheet to be the right sheet can easily get screwed up. Much harder to deliberately use the wrong sheet Name / Variable.

Ideally, your main sub will just be a list of other subs along the lines of:

```
DeleteVoidedTrades arg, arg, arg

finalRow = GetFinalRow(arg, arg) '/ functions required enclosed arguments

StackExchange Code Review Q#113369, answer score: 6