If I were to rewrite your code, I'd do it like this:
Option Explicit
Step one, always be explicit. Nobody likes seeing a variable being used that they never saw declared anywhere.
Public Sub ImportTextFile()
Step two, name the procedure after what it's doing, and make it do that.
Dim filePath As String
filePath = "filepath.txt"
Dim fileNumber As Long
fileNumber = FreeFile
FreeFile
will give you a file number that's ok to use. Hard-coding a file number is never a good idea, even if you're only using one file.
Dim inputData As String
Dim lineCount As Long
On Error Goto ErrHandler
ToggleWaitMode True, "Importing text file..."
On Error Goto ErrHandler
means if anything wrong happens (file not found, more data in the file than can fit on the worksheet, or whatever), we'll skip straight to an error-handling subroutine where we'll take good care of closing our file and toggle "wait mode" back off.
Open filePath For Input As #fileNumber
Do Until EOF(fileNumber)
Line Input #fileNumber, inputData
lineCount = lineCount + 1
WriteLineContentToActiveSheet inputData, lineCount
Loop
Close #fileNumber
So the ImportTextFile
procedure does nothing but reading the file's content. WriteLineContentToActiveSheet
will do the worksheet-writing part.
ErrHandler:
ToggleWaitMode
If Err.Number <> 0 Then
Close 'closes any file left open
MsgBox Err.Description
Err.Clear
End If
End sub
The code under this label will execute even if no error occurred, this is why we're checking for Err.Number
being other than 0. But regardless of whether there's an error or not, we always want to ToggleWaitMode
.
Private Sub ToggleWaitMode(Optional ByVal wait As Boolean = False, Optional ByVal statusBarMessage As String = vbNullString)
Application.ScreenUpdating = True 'ensure status message gets displayed
Application.StatusBar = statusBarMessage
Application.Cursor = IIf(wait, xlWait, xlDefault)
Application.ScreenUpdating = Not wait
Application.EnableEvents = Not Wait
End Sub
This small procedure takes two optional parameters. If none are specified, "wait mode" is toggled off - this means the status bar message is reset, the mouse cursor goes back to default, Excel resumes redrawing the screen and fires events whenever something happens. "Wait mode" is just all that reversed :)
Private Sub WriteLineContentToActiveSheet(ByVal inputData As String, ByVal targetRow As Long)
Dim lineFields() As String
lineFields = Split(inputData, "|")
Dim fieldCount As Integer
fieldCount = UBound(lineFields)
Dim currentField As Integer
Dim targetColumn As Integer
Dim fieldValue As String
With ActiveSheet
For currentField = 0 To fieldCount
fieldValue = lineFields(currentField)
targetColumn = fieldCount - currentField + 1
.Cells(targetRow, targetColumn).Value = fieldValue
Next
End With
End Sub
This small procedure takes a single line of data, splits it into as many fields as it has, and writes each field value into a cell of the active sheet. Using UBound
we're no longer tied to a specific number of fields - if the file format changes, the procedure should still work. And if it doesn't, the procedure that called it will cleanly handle the error for us.
Local variables currentField
, targetColumn
and fieldValue
are not necessary - they could all be inlined, but having them makes code easier to follow and allows you to place breakpoints to validate their values as you run the code line-by-line with F8 when you're debugging.
Couple observations:
- You need to indent your code. Indentation makes it much easier to read, for yourself and anyone looking at it. Give that Tab button some lovin'!
- Procedures define a scope, anything under it should be indented.
- Within a scope, code blocks should also be indented - this includes
If
blocks, With
blocks and any code that runs inside a loop.
OpenTextFile
is a bad name for a procedure that actually imports a text file.
- Be consistent with your naming: chose a convention, and stick to it.
- You're reading a file, that's an I/O operation and by Murphy's Law this is going to fail one day or another. Make sure your code properly handles any error that could happen while processing the file, you don't want to leave a file handle opened by mistake.
- Importing a 20mb text file will take a while, even with screen updating turned off. Tell your user you're working, give'em a hourglass cursor, and you can even update the statusbar message every couple thousand lines read/written (just call
ToggleWaitMode True
with a new status message) so they know it's not frozen.
Application.ScreenUpdating
toFalse
and then back toTrue
once you're done with the file? – Mat's Mug♦ Nov 3 '13 at 5:59