# VBA Data Conversion (random printout -> CSV file)

I have reviewed the FAQ and I believed it was yes to all 5 q's of "What questions are on-topic for this site?"

Basically, the code below is used to perform the operation shown in the picture. I'm wondering what I can improve in the attached code to clean it up for future reference. Thanks in advance!

Sub Button1_Click()

Dim orgFilename As String
Dim temp As String
Dim strarray(3) As String
Dim vert(4) As String
Dim vert2(3) As String
Dim newFilename As String
Dim numRows As Integer
Dim i As Integer
Dim j As Integer
Dim k As Integer
Dim segCount As Integer
Dim vertex(3, 100) As Double

Dim oldwb As Workbook
Dim newwb As Workbook

orgFilename = Application.GetOpenFilename(FileFilter:="All files (*.), *.", Title:="Please select a file")
If orgFilename = "False" Then Exit Sub
Workbooks.OpenText Filename:=orgFilename, _
Origin:=950, StartRow:=1, DataType:=xlDelimited, TextQualifier:= _
xlDoubleQuote, ConsecutiveDelimiter:=True, Tab:=True, Semicolon:=False, _
Comma:=False, Space:=True, Other:=False, FieldInfo:=Array(Array(1, 1), _
Array(2, 1), Array(3, 1)), TrailingMinusNumbers:=True
Set oldwb = ActiveWorkbook

oldwb.Activate
Cells(5, 1).Select
numRows = Cells(5, 1).End(xlDown).Row

' Parse through data
segCount = 0
j = 1
For i = 5 To numRows
If Cells(i, 1) <> "VRTX" And segCount <> 0 Then
For k = 1 To segCount - 1
newwb.Worksheets("Sheet1").Cells(j, 1) = "GLINE"
With newwb.Worksheets("Sheet1")
.Cells(j, 2) = vertex(1, k)
.Cells(j, 3) = vertex(3, k)
.Cells(j, 4) = vertex(2, k)
.Cells(j, 5) = vertex(1, k + 1)
.Cells(j, 6) = vertex(3, k + 1)
.Cells(j, 7) = vertex(2, k + 1)
End With
j = j + 1
Next k
segCount = 0
ElseIf Cells(i, 1) = "VRTX" Then
' Save vertices to save an endpoint
vertex(1, segCount + 1) = Cells(i, 3)
vertex(2, segCount + 1) = Cells(i, 4)
vertex(3, segCount + 1) = Cells(i, 5)
segCount = segCount + 1
End If
Next i

' Save as a new file
temp = Mid$(orgFilename, InStrRev(orgFilename, "\") + 1) temp = Replace$(temp, ".pl", ".csv")
strarray(1) = Left(orgFilename, InStrRev(orgFilename, "\"))
strarray(2) = "processed_"
strarray(3) = temp
newFilename = Join(strarray, "")
newwb.SaveAs Filename:=newFilename, _
FileFormat:=xlCSV, _
CreateBackup:=False
End Sub


Your code does not look bad overall. Here are my 2 cents:

1. To improve readability, I would split your procedure in sub procedures / functions (does not make a difference if this is the only function in the module, but makes a difference in larger projects). The main procedure could look like this for example (simplified):

Sub main()
openPlFile
writeCsvFile
saveCsvFile
End Sub

2. You could add some error handling logic:

On Error GoTo error_handler


And at the end of the code:

Exit Sub
error_handler:
'code to handle the error for example:
MsgBox "There was an error: " & Err.Description
End Sub

3. If performance is an issue there are a few steps to improve the code:

a. You can turn off ScreenUpdating and turn Calculation to manual (don't forget to turn them back on before exiting AND in the error_handler block if there is one).

b. If you want to improve performance further, you could use arrays instead of reading from / writing to the spreadsheet directly in a loop (see http://www.avdf.com/apr98/art_ot003.html)

c. If you want to improve performance even more, you could read and write the files directly without opening / creating workbooks.

d. I generally avoid using Integer as they use the same memory space as Longs anyway (on 32 or 64 bit systems) - VBA simply applies different overflow rules. Declaring Longs instead of Integers can result in a slight performance gain (not so much in your case, but can be useful when declared in a loop for example)

• Regarding 'VBA automatically translates them into Long' this is certainly not the case. If you do Dim Foo as Integer and then try to use it as a long (say, loop through a million rows with it) you will get an Overflow error since an Integer can only hold numbers between -32,768 to 32,768 whereas a Long can hold -2,147,483,648 to 2,147,483,648. The benefit of using Longs is that you are highly unlikely to ever encounter a situation where you reach an overflow error with it (and if you do hit the cap, chances are your code is bad). – Brandon Barney Jul 26 '17 at 11:25
• As far as I am aware, there is no performance gain with Longs (they do use more memory after all) but they are much safer than Integers. – Brandon Barney Jul 26 '17 at 11:26
• @BrandonBarney If you are using a 32+ bit system, which is very likely to be the case nowadays, Integers effectively use 32 bits although the overflow rules for integers are applied (i.e. behaves as if it were a 16 bit variable). See for example: stackoverflow.com/questions/26717148/integer-vs-long-confusion – assylias Jul 26 '17 at 13:31
• @BrandonBarney I've amended that section of my answer to clarify. – assylias Jul 26 '17 at 13:33
• I would argue that, while on the backend you are correct, suggesting that the Integer is being converted to a Long is misleading (case in point, the OP of the question you posted). While the memory used for the Integer is different, for all intents and purposes it is an integer and not a Long. Most users (especially users in the position of the OP) will have no foundation for even understanding how this makes a difference for their code. – Brandon Barney Jul 26 '17 at 13:35

Some pointers off the top of my head:

1. It's always good practice to have good variable names: i, j, and k aren't bad but they could be more explicit. (for example, i could be currentRowIndex)

2. "VRTX" would be better off as a string constant: Const S_VRTX As String = "VRTX"

3. I personally like to even out anything like this:

Workbooks.OpenText Filename:=orgFilename, _
Origin:=950, StartRow:=1, DataType:=xlDelimited, TextQualifier:= _
xlDoubleQuote, ConsecutiveDelimiter:=True, Tab:=True, Semicolon:=False, _
Comma:=False, Space:=True, Other:=False, FieldInfo:=Array(Array(1, 1), _
Array(2, 1), Array(3, 1)), TrailingMinusNumbers:=True


to something like this:

Workbooks.OpenText Filename:=orgFilename, _
Origin:=950, _
StartRow:=1, _
DataType:=xlDelimited, _
TextQualifier:= xlDoubleQuote, _
ConsecutiveDelimiter:=True, _
Tab:=True, _
Semicolon:=False, _
Comma:=False, _
Space:=True, _
Other:=False, _
FieldInfo:=Array(Array(1, 1), Array(2, 1), Array(3, 1)), _
TrailingMinusNumbers:=True

4. It might be good to check that you have the requisite amount of data (e.g. 5 rows, 4 columns) before accessing it - say you open a file without the right amount of data, it won't handle it well in almost any language.

5. Turn on Option Explicit and you'll find some things can tell you in advance how to improve themselves :)

• Thanks for the feedback. I never learned VBA properly, let alone haven't used it in this depth for 3 years. Really interesting points to learn for the next project. – stanigator Feb 8 '12 at 4:05

The code below is in two parts. I have pasted code with your old code commented out, and then additional comments for clarity. I have also posted a cleaner version with just comments.

Note: From reading the code, it likely wont run properly. I was unable to figure out exactly what your loop is doing, and as a result there are still some bugs. Be sure to fully debug this code before using it.

## Full Version

Option Explicit
Sub Button1_Click()
' Constants are declared at the beginning of the routine.
Const ROW_SKIP As Long = 5

' Avoid Dim blocks like these. It is always best to declare variables as close to their initial use
' as possible. This makes your code easier to read/maintain as well.
'Dim orgFilename As String
'Dim temp As String
'Dim strarray(3) As String
'Dim vert(4) As String
'Dim vert2(3) As String
'Dim newFilename As String
'Dim numRows As Integer
'Dim i As Integer
'Dim j As Integer
'Dim k As Integer
'Dim segCount As Integer
'Dim vertex(3, 100) As Double
'
'Dim oldwb As Workbook
'Dim newwb As Workbook

' I will declare the variable name, but I will also use a name that is slightly more descriptive.
' This will allow others to understand what I am doing. I also encapsulate this in a function to allow for
' easy error handling.
'orgFilename = Application.GetOpenFilename(FileFilter:="All files (*.), *.", Title:="Please select a file")

' Instead of just exiting the sub, handle this error.
' If orgFilename = "False" Then Exit Sub

Dim InputFileName As String
InputFileName = GetInputFileName

If InputFileName = vbNullString Then
' We can add a messagebox here if needed. For now, we just exit the routine silently.
Exit Sub
End If

' For your field info here, you are using an uninitialized, undeclared, array. What effect are you intending to achieve?
Workbooks.OpenText _
Filename:=orgFilename, _
Origin:=950, _
StartRow:=1, _
DataType:=xlDelimited, _
TextQualifier:= _
xlDoubleQuote, _
ConsecutiveDelimiter:=True, _
Tab:=True, Semicolon:=False, _
Comma:=False, _
Space:=True, _
Other:=False, _
FieldInfo:=Array(Array(1, 1), Array(2, 1), Array(3, 1)), _
TrailingMinusNumbers:=True

' I declare more descriptive workbook variable names, and separate the assignments.
' Set oldwb = ActiveWorkbook

' I am changing this to a Sheet reference since you seem to be referring to the ActiveSheet implicitly, and not just the ActiveWorkbook
Dim CurrentWorksheet As Worksheet
Set CurrentWorksheet = ActiveSheet

' While the default scope of Workbooks.Add is Application.Workbooks.Add it is better to be explicit.
Dim OutputWorkbook As Workbook

' No need for Activate. Try to avoid this behavior.
' oldwb.Activate

' Avoid Select as well
' Cells(5, 1).Select
' numRows = Cells(5, 1).End(xlDown).Row

' Declare new variable, and qualify the range reference when finding the row. Without the qualifying reference
' to CurrentWorkbook the Cells reference refers to the ActiveWorkbook.
Dim NumberOfRows As Long
NumberOfRows = CurrentWorksheet.Cells(5, 1).End(xlDown).Row

' Instead of making changed within the loop, I am just going to rewrite it to make changes easier to read.
' Parse through data
'segCount = 0
'j = 1
'For i = 5 To numRows
'    If Cells(i, 1) <> "VRTX" And segCount <> 0 Then
'        For k = 1 To segCount - 1
'            newwb.Worksheets("Sheet1").Cells(j, 1) = "GLINE"
'            With newwb.Worksheets("Sheet1")
'                .Cells(j, 2) = vertex(1, k)
'                .Cells(j, 3) = vertex(3, k)
'                .Cells(j, 4) = vertex(2, k)
'                .Cells(j, 5) = vertex(1, k + 1)
'                .Cells(j, 6) = vertex(3, k + 1)
'                .Cells(j, 7) = vertex(2, k + 1)
'            End With
'            j = j + 1
'        Next k
'        segCount = 0
'    ElseIf Cells(i, 1) = "VRTX" Then
'        ' Save vertices to save an endpoint
'        vertex(1, segCount + 1) = Cells(i, 3)
'        vertex(2, segCount + 1) = Cells(i, 4)
'        vertex(3, segCount + 1) = Cells(i, 5)
'        segCount = segCount + 1
'    End If
'Next i

' Assumes that the UsedRange of the Input sheet is the data we need
Dim InputData As Variant
InputData = CurrentWorksheet.UsedRange.Value

Dim SegmentCount As Long

Dim i As Long
Dim j As Long
Dim k As Long

j = 1

' Re-creating your vertex array though it is not at all clear what it is being used for.
Dim Vertices As Variant
ReDim Vertices(3, 100)

' I use a constant variable instead of 5 here since the 5 may change, and it can be difficult to track it down later.
' Note: This will always return false on the first pass since SegmentCount will always equal 0
If InputData(i, 1) <> "VRTX" And SegmentCount <> 0 Then
For k = 1 To segCount - 1
OutputWorkbook.Worksheets("Sheet1").Cells(j, 1) = "GLINE"
With OutputWorkbook.Worksheets("Sheet1")
.Cells(j, 2) = Vertices(1, k)
.Cells(j, 3) = Vertices(3, k)
.Cells(j, 4) = Vertices(2, k)
.Cells(j, 5) = Vertices(1, k + 1)
.Cells(j, 6) = Vertices(3, k + 1)
.Cells(j, 7) = Vertices(2, k + 1)
End With
j = j + 1
Next k
SegmentCount = 0
ElseIf InputData(i, 1) = "VRTX" Then
Vertices(1, SegmentCount + 1) = InputData(i, 3)
Vertices(2, SegmentCount + 1) = InputData(i, 4)
Vertices(3, SegmentCount + 1) = InputData(i, 5)

SegmentCount = SegmentCount + 1
End If
Next i

' This can be condensed into a much more concise format

' Save as a new file
' temp = Mid$(orgFilename, InStrRev(orgFilename, "\") + 1) ' temp = Replace$(temp, ".pl", ".csv")

' strarray(1) = Left(orgFilename, InStrRev(orgFilename, "\"))
' strarray(2) = "processed_"
' strarray(3) = temp

' newFilename = Join(strarray, "")

Dim OutputFileName As String

' This takes care of the entire operation in one line, and allows others to see what these operations are being used for.
OutputFileName = Left(orgFilename, InStrRev(orgFilename, "\")) & "processed_" & Replace$(Mid$(orgFilename, InStrRev(orgFilename, "\") + 1), ".pl", ".csv")

OutputWorkbook.SaveAs Filename:=OutputFileName, _
FileFormat:=xlCSV, _
CreateBackup:=False
End Sub
Private Function GetInputFileName() As String
' I use a variant declaration because the return of Cancel is the Boolean false.
Dim InputFileNameResult As Variant

InputFileNameResult = Application.GetOpenFilename(FileFilter:="All files (*.), *.", Title:="Please select a file")

If Not InputFileNameResult Then
GetInputFileName = InputFileNameResult
Else
' You can handle this as needed. For now, we just assume the user wants to exit the routine.
' As such, we do nothing.
End If
End Function


## Condensed Version

Option Explicit
Sub Button1_Click()
' Constants are declared at the beginning of the routine.
Const ROW_SKIP As Long = 5

Dim InputFileName As String
InputFileName = GetInputFileName

If InputFileName = vbNullString Then
' We can add a messagebox here if needed. For now, we just exit the routine silently.
Exit Sub
End If

' For your field info here, you are using an uninitialized, undeclared, array. What effect are you intending to achieve?
Workbooks.OpenText _
Filename:=orgFilename, _
Origin:=950, _
StartRow:=1, _
DataType:=xlDelimited, _
TextQualifier:= _
xlDoubleQuote, _
ConsecutiveDelimiter:=True, _
Tab:=True, Semicolon:=False, _
Comma:=False, _
Space:=True, _
Other:=False, _
FieldInfo:=Array(Array(1, 1), Array(2, 1), Array(3, 1)), _
TrailingMinusNumbers:=True

' I am changing this to a Sheet reference since you seem to be referring to the ActiveSheet implicitly, and not just the ActiveWorkbook
Dim CurrentWorksheet As Worksheet
Set CurrentWorksheet = ActiveSheet

' While the default scope of Workbooks.Add is Application.Workbooks.Add it is better to be explicit.
Dim OutputWorkbook As Workbook

' Declare new variable, and qualify the range reference when finding the row. Without the qualifying reference
' to CurrentWorkbook the Cells reference refers to the ActiveWorkbook.
Dim NumberOfRows As Long
NumberOfRows = CurrentWorksheet.Cells(5, 1).End(xlDown).Row

' Assumes that the UsedRange of the Input sheet is the data we need
Dim InputData As Variant
InputData = CurrentWorksheet.UsedRange.Value

Dim SegmentCount As Long

Dim i As Long
Dim j As Long
Dim k As Long

j = 1

' Re-creating your vertex array though it is not at all clear what it is being used for.
Dim Vertices As Variant
ReDim Vertices(3, 100)

' I use a constant variable instead of 5 here since the 5 may change, and it can be difficult to track it down later.
' Note: This will always return false on the first pass since SegmentCount will always equal 0
If InputData(i, 1) <> "VRTX" And SegmentCount <> 0 Then
For k = 1 To segCount - 1
OutputWorkbook.Worksheets("Sheet1").Cells(j, 1) = "GLINE"
With OutputWorkbook.Worksheets("Sheet1")
.Cells(j, 2) = Vertices(1, k)
.Cells(j, 3) = Vertices(3, k)
.Cells(j, 4) = Vertices(2, k)
.Cells(j, 5) = Vertices(1, k + 1)
.Cells(j, 6) = Vertices(3, k + 1)
.Cells(j, 7) = Vertices(2, k + 1)
End With
j = j + 1
Next k
SegmentCount = 0
ElseIf InputData(i, 1) = "VRTX" Then
Vertices(1, SegmentCount + 1) = InputData(i, 3)
Vertices(2, SegmentCount + 1) = InputData(i, 4)
Vertices(3, SegmentCount + 1) = InputData(i, 5)

SegmentCount = SegmentCount + 1
End If
Next i

' This takes care of the entire operation in one line, and allows others to see what these operations are being used for.
Dim OutputFileName As String
OutputFileName = Left(orgFilename, InStrRev(orgFilename, "\")) & "processed_" & Replace$(Mid$(orgFilename, InStrRev(orgFilename, "\") + 1), ".pl", ".csv")

OutputWorkbook.SaveAs Filename:=OutputFileName, _
FileFormat:=xlCSV, _
CreateBackup:=False
End Sub
Private Function GetInputFileName() As String
' I use a variant declaration because the return of Cancel is the Boolean false.
Dim InputFileNameResult As Variant

InputFileNameResult = Application.GetOpenFilename(FileFilter:="All files (*.), *.", Title:="Please select a file")

If Not InputFileNameResult Then
GetInputFileName = InputFileNameResult
Else
' You can handle this as needed. For now, we just assume the user wants to exit the routine.
' As such, we do nothing.
End If
End Function


## Overall Observations

There are a number of inefficiencies that are holding you back. First, you are still using Activate, Select and ActiveWorkbook. While there is a time and a place for each of these, it is preferable, by far, to avoid them. You can do this by fully qualifying your references.

SomeWorkbook.Activate
Sheets("SomeSheet").Select
msgbox Cells(1,10)


Becomes

msgbox SomeWorkbook.Sheets("SomeSheet").Cells(1,10).Value


This allows you greater control over your code, and reduces the risk of errors being raised.

For more detail on this, check out this link: https://stackoverflow.com/questions/10714251/how-to-avoid-using-select-in-excel-vba-macros .

## Dim Blocks

This is a lesser known rule, but is is an important one to learn as soon as possible. Not only are Dim blocks ugly they make it harder to see bad code.

For example, how many variables do you have declared but never used? What about used and never declared? These are both code smells (with the latter being very likely to cause an error).

To avoid this, declare your variables as close to their first use as possible. This comes with the caveat of not declaring them within Loops. Once you follow this practice, your code becomes easier to read and maintain.

## Variable Names

Variable names should be descriptive, but concise. This sometimes can make it difficult to find the right name, but descriptive names make a difference. For example newFileName is a decent name. We know what it is. On the other hand orgFileName is obscure. I have no clue what this means.

When writing code I tend to think of my procedures as having Inputs and Outputs so orgFileName becomes InputFileName and newFileName becomes OutputFileName. This changes a bit as you get into Functions Subs and Classes but, for in a simplified state this principle works fairly well.

Dont only every use Input and Output though. There are other situations such as segCount which is really a SegmentCount and numRows (fairly strong) which can be NumberOfRows.

Loop Iterators

Loop iterators get away with being able to be less descriptive. Using i, j, k, l, m, n... is generally fine (though if you need that many loop iterators, get help). Be forewarned though that l is difficult to distinguish from 1 and n is generally used in equations.

Loop Iterator Types

As noted in another answer, the type of Integer should pretty much never be used. That is due to the limit of an Integer which is ~32,000 whereas the limit of a long is ~2,000,000,000. Yeah, Long is able to handle much bigger numbers.

## Option Explicit

To say that Option Explicit is vital would be an understatement. I started using it, at the advice of others, and it literally has saved me countless hours of debugging for what would, otherwise be, stupid mistakes.

You can type Option Explicit manually at the beginning of every routine, or you can use this shortcut to change the setting:

1. ALT + T
2. ALT + O
3. In the Editor Tab, select the Require Variable Declarations checkbox.

## Rubberduck

One of the frequent contributors, Mat's Mug, is working on a project called Rubberduck. Him and his team have made it into an amazing tool that is capable of Code Inspections, Refactoring, some better UI elements, and some other really cool stuff. I strongly suggest checking it out here: http://rubberduckvba.com/ .

If you have any questions, please don't hesitate to ask. This is a lot to take in at once, but I would be doing you a disservice if I turned a blind eye to common mistakes.