# 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

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
colorImp = 23

Do
DoEvents

'Row color tester is a proxy for testing trade time "structure principle"
row_number = row_number + 1
row_color_tester = row_number

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

row_number = 3

Do
DoEvents

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

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

• what is the first row_number value? – Dirk Reichel Dec 9 '15 at 14:28
• @DirkReichel The first row_number value is 3. Rows 1 and 2 contain buttons and a date input cell (which is used by a separate macro used to store the table and data created by this macro in a newly created sheet). Thank you very much! – StormsEdge Dec 9 '15 at 14:34
• so the first row to check/delete is 4, but the check/highlight starts with row 3? – Dirk Reichel Dec 9 '15 at 14:46
• @Tensibai Thanks Tensibai! Appreciate it! I sat here like an idiot hitting space 4 times on each line hahaha – StormsEdge Dec 9 '15 at 15:38
• You're welcome, and your intro made me think about an "agile mantra": 'Make it works, Make it right, Make it fast', I.e: works on iteration, getting working code with tests to confirm you don't break it on next refactoring to make it cleaner and then faster. – Tensibai Dec 9 '15 at 15:40

## 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.

## 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.

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

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

Dim row As Long

For row = finalRow To 1 Step -1

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

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.

• This is a huge help! I will make some changes and commit your recommendations to memory. Thanks so much! – StormsEdge Dec 21 '15 at 18:45

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
crng = .Range("M1:M" & lastrow).Text

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
crng = .Range("B1:B" & lastrow + 1).Text

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)

• Thank you @DirkReichel Definitely gives me a better idea of how to work some things. – StormsEdge Dec 9 '15 at 15:40