VBA Data Conversion (random printout -> CSV file)

Question

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! enter image description here

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
Set newwb = Workbooks.Add
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, _
 Password:="", WriteResPassword:="", ReadOnlyRecommended:=False, _
 CreateBackup:=False
End Sub

Answer 1

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
    readPlFile
    writeCsvFile
    saveCsvFile
   End Sub
   

2. You could add some error handling logic:

   Right after your declarations:

   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)

• Brandon Barney: 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: 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.
• assylias: @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: @BrandonBarney I've amended that section of my answer to clarify.
• Brandon Barney: 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.

Answer 2

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 :)

• stanigator: 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.

Answer 3

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
 ' Set newwb = Workbooks.Add
 ' 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
 Set OutputWorkbook = Application.Workbooks.Add
 ' 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.
 For i = ROW_SKIP To NumberOfRows
 ' 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, _
 Password:="", _
 WriteResPassword:="", _
 ReadOnlyRecommended:=False, _
 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
 Set OutputWorkbook = Application.Workbooks.Add
 ' 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.
 For i = ROW_SKIP To NumberOfRows
 ' 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, _
 Password:="", _
 WriteResPassword:="", _
 ReadOnlyRecommended:=False, _
 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.