patternModerate
Converting from .xlsx to .txt
Viewed 0 times
convertingfromxlsxtxt
Problem
I have code that converts each sheet of a spreadsheet into a .txt file. It works well, however, given the big number of exports (about 90 .txt files), I'd like to seek advice on how to speed it up.
Sub xlsxTotxt()
Dim i As Integer
Dim directory As String
Dim fname As String
Dim xWs As Worksheet
Dim xTextFile As String
Dim rdate As String
directory = ThisWorkbook.Sheets("Macro").Range("D576").Value
rdate = ThisWorkbook.Sheets("Macro").Range("E47").Value
i = 0
Application.ScreenUpdating = False
Application.DisplayAlerts = False
Do While ThisWorkbook.Sheets("Macro").Range("D577").Offset(i).Value <> ""
fname = Sheets("Macro").Range("D577").Offset(i).Value
Workbooks.Open (directory & fname)
For Each xWs In Workbooks(fname).Worksheets
xWs.Copy
xTextFile = directory & rdate & " - " & xWs.name & ".txt"
Application.ActiveWorkbook.SaveAs filename:=xTextFile, FileFormat:=xlText
Application.ActiveWorkbook.Saved = True
Application.ActiveWorkbook.Close
Next
Workbooks(fname).Close
i = i + 1
Loop
Application.ScreenUpdating = True
Application.DisplayAlerts = True
End SubSolution
Your code is incredibly fragile
If a single row/column gets added or deleted or moved, those ranges are going to move and your code is going to fail completely.
If you can, those values should be in a dedicated sheet, not buried below 500 lines of other stuff. If not, name your ranges.
So, let's say you take cell
which is incredibly fragile, you can use
and so long as nobody actually deletes said row/column, that will always point to the right place.
Use
Rather than re-referencing the same object over and over, you can use a
Becomes
much clearer and easier to read.
Handle things in the right order
Specifically your
Use descriptive naming
Code should be written for other people (including future you) to read. Names should be descriptive, unambiguous and concise. In that order. I highly recommend the excellent, classic article on naming by Joel Spolsky. But in short, things should sound like what they are.
Also, move your declarations near to where they're used. This helps you see the different parts of your method and helps you keep track of where you are, refer back to your declarations and see ways to split things up into logical sub-methods. Personally, I prefer to keep declarations outside of Loop structures, but that's personal preference.
Objects are your friend
VBA has a huge, extensive, comprehensive object model for every Office Application. Use it.
You refer to that sheet multiple times. Rather than continually repeating that reference, put it in a
Same with
Relying on
Indenting
Indenting is a wonderful way to let you see the stru
directory = ThisWorkbook.Sheets("Macro").Range("D576").Value
rdate = ThisWorkbook.Sheets("Macro").Range("E47").ValueIf a single row/column gets added or deleted or moved, those ranges are going to move and your code is going to fail completely.
If you can, those values should be in a dedicated sheet, not buried below 500 lines of other stuff. If not, name your ranges.
So, let's say you take cell
D576 and name it Directory_Path or something similar. Now, rather thandirectory = ThisWorkbook.Sheets("Macro").Range("D576").Valuewhich is incredibly fragile, you can use
directory = ThisWorkbook.Range("Directory_Path").Valueand so long as nobody actually deletes said row/column, that will always point to the right place.
Use
WithRather than re-referencing the same object over and over, you can use a
With statement to hold a reference. Like so:Application.ActiveWorkbook.SaveAs Filename:=xTextFile, FileFormat:=xlText
Application.ActiveWorkbook.Saved = True
Application.ActiveWorkbook.CloseBecomes
With Application.ActiveWorkbook
.SaveAs Filename:=xTextFile, FileFormat:=xlText
.Saved = True
.Close
End Withmuch clearer and easier to read.
Handle things in the right order
Specifically your
Application.[Settings]. Anything meta like that should go right at the start and right at the end of the method(s) it applies to. This allows you to confirm, at a glance, what the internal state of your method is and check that things are set/reset correctly. You should also disable Application.EnableEvents and Application.Calculation for significant extra speed. Like so:Sub xlsxTotxt()
With Application
.ScreenUpdating = False
.DisplayAlerts = False
.EnableEvents = False
.Calculation = xlCalculationManual
End With
...
Code
...
With Application
.ScreenUpdating = True
.DisplayAlerts = True
.EnableEvents = True
.Calculation = xlCalculationAutomatic
End With
End SubUse descriptive naming
Code should be written for other people (including future you) to read. Names should be descriptive, unambiguous and concise. In that order. I highly recommend the excellent, classic article on naming by Joel Spolsky. But in short, things should sound like what they are.
xTextFile. What on earth is that? Looking at it I have no idea. If I had to guess, I'd say it's some kind of file object. Oh, it's the filename you want to save your text file under? Why not call it newFilename? Since it's a full filename (including directory path). Maybe newFullfilename or just fullFilename might be even more descriptive.xWs suffers from the same problem. I can guess it's a worksheet object but beyond that? no idea. Since you just use it to iterate over the sheets in your workbook, maybe just call it currentSheet?fname. Same thing. filename. Since it's the filename for the workbook you're opening, how about targetWorkbookFilename? Sure, it's long, but screen real estate is cheap, cognitive processing is not and that name's an awful lot easier to understand and work with.Also, move your declarations near to where they're used. This helps you see the different parts of your method and helps you keep track of where you are, refer back to your declarations and see ways to split things up into logical sub-methods. Personally, I prefer to keep declarations outside of Loop structures, but that's personal preference.
Objects are your friend
VBA has a huge, extensive, comprehensive object model for every Office Application. Use it.
ThisWorkbook.Sheets("Macro")You refer to that sheet multiple times. Rather than continually repeating that reference, put it in a
worksheet object and then just refer to the object.Dim macroSheet As Worksheet
Set macroSheet = ThisWorkbook.Sheets("Macro")
Dim dateString As String
dateString = macroSheet.Range("E47").ValueSame with
Workbooks.Do While ThisWorkbook.Sheets("Macro").Range("D577").Offset(i).Value <> ""
fname = Sheets("Macro").Range("D577").Offset(i).Value
Workbooks.Open (directory & fname)
For Each xWs In Workbooks(fname).Worksheets
xWs.Copy
xTextFile = directory & rdate & " - " & xWs.name & ".txt"
Application.ActiveWorkbook.SaveAs filename:=xTextFile, FileFormat:=xlText
Application.ActiveWorkbook.Saved = True
Application.ActiveWorkbook.CloseRelying on
ActiveWorkbook being the one you want is very fragile. Just make it a proper object and then your references will always be accurate:Dim targetWorkbook As Workbook
Set targetWorkbook = Workbooks.Open (directory & fname)
...
For Each currentSheet In targetWorkbook.Sheets()
...
With targetWorkbook
.SaveAs filename:=xTextFile, FileFormat:=xlText
.Saved = True
.Close
End WithIndenting
Indenting is a wonderful way to let you see the stru
Code Snippets
directory = ThisWorkbook.Sheets("Macro").Range("D576").Value
rdate = ThisWorkbook.Sheets("Macro").Range("E47").Valuedirectory = ThisWorkbook.Sheets("Macro").Range("D576").Valuedirectory = ThisWorkbook.Range("Directory_Path").ValueApplication.ActiveWorkbook.SaveAs Filename:=xTextFile, FileFormat:=xlText
Application.ActiveWorkbook.Saved = True
Application.ActiveWorkbook.CloseWith Application.ActiveWorkbook
.SaveAs Filename:=xTextFile, FileFormat:=xlText
.Saved = True
.Close
End WithContext
StackExchange Code Review Q#132826, answer score: 11
Revisions (0)
No revisions yet.