Tell me more ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

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
share|improve this question

2 Answers

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 VBA automatically translates them into Long. So declaring Longs instead of Integers is a slight performance gain (not so much in your case, but can be useful when declared in a loop for example)

share|improve this answer
+1 I did not know that about Ints and Longs... – Gaffi Mar 15 '12 at 19:22

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

share|improve this answer
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

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.