Raw exchange trade data analysis program

Question

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).Text

If tradeTime = Range("B" & (row_color_tester + 1)).Text Or tradeTime = Range("B" & (row_color_tester - 1)).Text Then
    Range("A" & row_number, "O" & row_number).Interior.ColorIndex = colorImp

End If

Loop Until row_number = lastRow

'Analyze trade data for structures
row_number = 3
tradeMonth = Range("F" & row_number).Text

Do
DoEvents

tradeTime = ActiveSheet.Range("B" & row_number).Text
legCounter = 0

 'Counts legs to narrow structure type
 Do
 DoEvents
    row_color_tester = row_number

    If tradeTime = Range("B" & (row_color_tester + 1)).Text Or tradeTime = Range("B" & (row_color_tester - 1)).Text Then
        legCounter = legCounter + 1

    End If

    row_number = row_number + 1

Loop Until tradeTime <> Range("B" & (row_color_tester + 1)).Text

' Following large IF Statements analyze individual legs to determine specific structure types
If legCounter = 2 Then
    Range("M" & (row_number - legCounter)) = legCounter
    'Declaring variables for string interpolation
    Dim fLeg As String
    Dim sLeg As String
    Dim struct As String
    Dim mulLot As Boolean

    mulLot = False

    'Defining what the first leg value is
    fLeg = monthCode(Range("F" & (row_number - 1)).Text)

    'Defining second leg value
    sLeg = monthCode(Range("F" & (row_number - 2)).Text)

    If fLeg = sLeg And Range("J" & (row_number - 1)) = Range("J" & (row_number - 2)) Then
        mulLot = True
    End If
    'If statement determining strip or spread
    If Range("J" & (row_number - 1)) <> Range("J" & (row_number - 2)) Then
        struct = "Spread"

    ElseIf Range("J" & (row_number - 1)) = Range("J" & (row_number - 2)) Then
        struct = "Strip"
    End If

    If mulLot = False Then
        Range("O" & (row_number - legCounter)) = fLeg & "/" & sLeg & " " & struct
        Range("A" & (row_number - legCounter), "O" & (row_number - legCounter)).Interior.ColorIndex = 4
    ElseIf mulLot = True Then
        Range("O" & (row_number - legCounter)) = fLeg & " Multiple clips"
        Range("A" & (row_number - legCounter), "O" & (row_number - legCounter)).Interior.ColorIndex = 3
    End If

Answer 1

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

ColourStructuredPrincipleTradeRows finalRow, arg, arg

AnalyseTradeStructures finalRow, arg, arg

etc.

Which may, in turn, be made up of more Procedures. Notice how comments are completely unnecessary in the above code? That's called self-documenting code. Comments should never be required to say "What" the code is doing, only ever the "Why".

That is the end Goal of Refactoring, and it's how you can build bigger and bigger projects, by splitting it into smaller and smaller logical units until you get to a manageable size.

---

Tips and Tricks

---

Range("J" & (row_number - 1))

is better expressed as

Cells(row_number - 1, 10)

---

Do Events

Is only necessary for long series of operations. Generally, if I'm doing a big loop, I'll add something like this:

If loopCounter mod 10000 = 0 then Do Events

Which allows the program to *occasionally* relinquish control to the OS, without doing it so often as to impact run speed.

Answer 2

For the first part to improve the speed:

Dim i As Long, rng As Range, crng As Variant
With ActiveSheet

  'set values for next step
  lastrow = ThisWorkbook.Sheets("Trade_Data_Insert").Cells(Rows.Count, 1).End(xlUp).Row
  crng = .Range("M1:M" & lastrow).Text

  'Deletes voided trades
  For i = 3 To lastrow
    If InStr(crng(i, 1), "Void") Then
      If rng Is Nothing Then Set rng = .Rows(i) Else Set rng = Union(rng, .Rows(i))
    End If
  Next
  If Not rng Is Nothing Then rng.Delete

  'set values for next step
  Set rng = Nothing
  lastrow = ThisWorkbook.Sheets("Trade_Data_Insert").Cells(Rows.Count, 1).End(xlUp).Row
  crng = .Range("B1:B" & lastrow + 1).Text

  'Highlights trade structures via time
  For i = 3 To lastrow
    If crng(i, 1) = crng(i + 1, 1) Or crng(i, 1) = crng(i - 1, 1) Then
      If rng Is Nothing Then Set rng = Range("A" & i & ":O" & i) Else Set rng = Union(rng, Range("A" & i & ":O" & i))
    End If
  Next
  If Not rng Is Nothing Then rng.Interior.ColorIndex = 23

End With

It will delete the rows in one step (should be much faster) and also color the ranges in one step. Also pushing the ranges to check in a variable increases the speed. Still, I don't know why/where you want to use something like Set r = Range("H" & row_number)...

At least, I hope that you can get some ideas from the way I changed your code. Having a sample/copy of the full code may change a lot... feel free to post a link. (But keep in mind to not share secret information or something like that)