patternMinor
Converting CSV files to Excel workbooks
Viewed 0 times
workbooksexcelcsvfilesconverting
Problem
My converter converts all CSV files in the subfolders of folders 1, 2 & 3 into Excel workbooks. As of now, I am converting using codes for each folders. I previously tried to combine those into one using for loop, but an error occurred, so I've rolled back to the working code I had before the loop.
Can anyone show me how to clean this up with a loop or another method?
```
Private Sub CommandButton1_Click()
Dim CSVfolder As String, CSVfolder1 As String, CSVfolder2 As String
Dim fname, fname1, fname2 As String
Dim wBook As Workbook
Dim colSF As Collection
Dim vFile, vFile1, vFile2
Dim bHadFiles As Boolean
CSVfolder = "C:\Charts\1\"
CSVfolder1 = "C:\Charts\2\"
CSVfolder2 = "C:\Charts\3\"
Set colSF = GetSubFolders(CSVfolder)
For Each vFile In colSF
fname = Dir(CSVfolder & vFile & "\" & "*.csv")
Do While fname <> ""
bHadFiles = True
Application.ScreenUpdating = False
Set wBook = Workbooks.Open(CSVfolder & vFile & "\" & fname, Format:=6, Delimiter:=",")
wBook.SaveAs CSVFolder & vFile & "\" & Replace(fname, ".csv", ""), xlOpenXMLWorkbook
Application.CutCopyMode = False
wBook.Close False
fname = Dir()
Loop
If bHadFiles Then Kill CSVfolder & vFile & "\" & "*.csv"
Next
Set colSF = GetSubFolders(CSVfolder1)
For Each vFile1 In colSF
fname1 = Dir(CSVfolder1 & vFile1 & "\" & "*.csv")
Do While fname1 <> ""
bHadFiles = True
Application.ScreenUpdating = False
Set wBook = Workbooks.Open(CSVfolder1 & vFile1 & "\" & fname1, Format:=6, Delimiter:=",")
wBook.SaveAs CSVFolder1 & vFile1 & "\" & Replace(fname1, ".csv", ""), xlOpenXMLWorkbook
Application.CutCopyMode = False
wBook.Close False
fname1 = Dir()
Loop
If bHadFiles Then Kill CSVfolder1 & vFile1 & "\" & "*.csv"
Next
Set colSF = GetSubFolders(CSVfolder2)
For Each vFile2 In colSF
fname2 = Dir(CSVfolder2 & vFile2 & "\" & "*.csv")
Do While fname2 <> ""
Application.ScreenUpdating = False
Set wBook = Workbooks.Open(CSVfolder2 & vFile2 & "\" & fname2, Format:=
Can anyone show me how to clean this up with a loop or another method?
```
Private Sub CommandButton1_Click()
Dim CSVfolder As String, CSVfolder1 As String, CSVfolder2 As String
Dim fname, fname1, fname2 As String
Dim wBook As Workbook
Dim colSF As Collection
Dim vFile, vFile1, vFile2
Dim bHadFiles As Boolean
CSVfolder = "C:\Charts\1\"
CSVfolder1 = "C:\Charts\2\"
CSVfolder2 = "C:\Charts\3\"
Set colSF = GetSubFolders(CSVfolder)
For Each vFile In colSF
fname = Dir(CSVfolder & vFile & "\" & "*.csv")
Do While fname <> ""
bHadFiles = True
Application.ScreenUpdating = False
Set wBook = Workbooks.Open(CSVfolder & vFile & "\" & fname, Format:=6, Delimiter:=",")
wBook.SaveAs CSVFolder & vFile & "\" & Replace(fname, ".csv", ""), xlOpenXMLWorkbook
Application.CutCopyMode = False
wBook.Close False
fname = Dir()
Loop
If bHadFiles Then Kill CSVfolder & vFile & "\" & "*.csv"
Next
Set colSF = GetSubFolders(CSVfolder1)
For Each vFile1 In colSF
fname1 = Dir(CSVfolder1 & vFile1 & "\" & "*.csv")
Do While fname1 <> ""
bHadFiles = True
Application.ScreenUpdating = False
Set wBook = Workbooks.Open(CSVfolder1 & vFile1 & "\" & fname1, Format:=6, Delimiter:=",")
wBook.SaveAs CSVFolder1 & vFile1 & "\" & Replace(fname1, ".csv", ""), xlOpenXMLWorkbook
Application.CutCopyMode = False
wBook.Close False
fname1 = Dir()
Loop
If bHadFiles Then Kill CSVfolder1 & vFile1 & "\" & "*.csv"
Next
Set colSF = GetSubFolders(CSVfolder2)
For Each vFile2 In colSF
fname2 = Dir(CSVfolder2 & vFile2 & "\" & "*.csv")
Do While fname2 <> ""
Application.ScreenUpdating = False
Set wBook = Workbooks.Open(CSVfolder2 & vFile2 & "\" & fname2, Format:=
Solution
The very first thing we need to do is fix the indentation. If we can't read the code, we can't make it better. Everything inside of
You had the right idea with the loop. A loop will definitely clean this up immensely, but before we get to a loop, first let's extract a method to remove the duplication.
Note that all I did was move the code into it's own method and call it appropriately. There's still no loop, but now moving to a loop is both trivial and almost unnecessary. We like clean code around here though, so let's go ahead and do that.
I noticed you have this is a code behind, you may want to move this code to it's own module or class and call it from the click handler. That way your logic isn't bound up in the GUI where it can't be re-used. The only other thing to mention here is that if you're turning the screen updating off, then you must use an error handler to ensure that it always gets turned back on.
But we're not done yet, we extracted that method out, but left our mess hidden away in there. Let's clean it up too.
How many times do you need to concatenate this path together?
Yikes! Do it once before you enter the while loop.
No offense, but WTF is
Sub...End Sub should be one level in. Add another level when you enter an If, For, For Each, or Select.Sub Foo
' some code
Set colSF = GetSubFolders(CSVfolder)
For Each vFile In colSF
fname = Dir(CSVfolder & vFile & "\" & "*.csv")
Do While fname <> ""
bHadFiles = True
Application.ScreenUpdating = False
Set wBook = Workbooks.Open(CSVfolder & vFile & "\" & fname, Format:=6, Delimiter:=",")
wBook.SaveAs CSVfolder & vFile & "\" & Replace(fname, ".csv", ""), xlOpenXMLWorkbook
Application.CutCopyMode = False
wBook.Close False
fname = Dir()
Loop
If bHadFiles Then Kill CSVfolder & vFile & "\" & "*.csv"
Next
' more code
End SubYou had the right idea with the loop. A loop will definitely clean this up immensely, but before we get to a loop, first let's extract a method to remove the duplication.
Private Sub CommandButton1_Click()
Application.ScreenUpdating = False
Dim CSVfolder As String, CSVfolder1 As String, CSVfolder2 As String
CSVfolder = "C:\Charts\1\"
CSVfolder1 = "C:\Charts\2\"
CSVfolder2 = "C:\Charts\3\"
TransformFile CSVfolder
TransformFile CSVfolder1
TransformFile CSVfolder2
Application.ScreenUpdating = True
End Sub
Private Sub TransformFile(ByVal CSVfolder As String)
Dim fname As String
Dim vFile
Dim colSF As Collection
Dim wBook As Workbook
Set colSF = GetSubFolders(CSVfolder)
For Each vFile In colSF
fname = Dir(CSVfolder & vFile & "\" & "*.csv")
Do While fname <> ""
bHadFiles = True
Set wBook = Workbooks.Open(CSVfolder & vFile & "\" & fname, Format:=6, Delimiter:=",")
wBook.SaveAs CSVfolder & vFile & "\" & Replace(fname, ".csv", ""), xlOpenXMLWorkbook
Application.CutCopyMode = False
wBook.Close False
fname = Dir()
Loop
If bHadFiles Then Kill CSVfolder & vFile & "\" & "*.csv"
Next
End SubNote that all I did was move the code into it's own method and call it appropriately. There's still no loop, but now moving to a loop is both trivial and almost unnecessary. We like clean code around here though, so let's go ahead and do that.
Private Sub CommandButton1_Click()
Application.ScreenUpdating = False
Dim folders As New Collection
folders.Add "1"
folders.Add "2"
folders.Add "3"
'must be a variant in order to loop over a string collection
'using a string array instead of a collection is another good option
Dim CSVFolder As Variant
For Each CSVFolder In folders
TransformFile CSVFolder
Next
Application.ScreenUpdating = True
End SubI noticed you have this is a code behind, you may want to move this code to it's own module or class and call it from the click handler. That way your logic isn't bound up in the GUI where it can't be re-used. The only other thing to mention here is that if you're turning the screen updating off, then you must use an error handler to ensure that it always gets turned back on.
But we're not done yet, we extracted that method out, but left our mess hidden away in there. Let's clean it up too.
Private Sub TransformFile(ByVal CSVFolder As String)
Dim fname As String
Dim vFile
Dim colSF As Collection
Dim wBook As Workbook
Set colSF = GetSubFolders(CSVFolder)
For Each vFile In colSF
fname = Dir(CSVFolder & vFile & "\" & "*.csv")
Do While fname <> ""
bHadFiles = True
Application.ScreenUpdating = False
Set wBook = Workbooks.Open(CSVFolder & vFile & "\" & fname, Format:=6, Delimiter:=",")
wBook.SaveAs CSVFolder & vFile & "\" & Replace(fname, ".csv", ""), xlOpenXMLWorkbook
Application.CutCopyMode = False
wBook.Close False
fname = Dir()
Loop
If bHadFiles Then Kill CSVFolder & vFile & "\" & "*.csv"
Next
End SubHow many times do you need to concatenate this path together?
CSVFolder & vFile & "\" & "*.csv"Yikes! Do it once before you enter the while loop.
For Each vFile In colSF
filePath = CSVFolder & vFile & "\"
fname = Dir(filePath & "*.csv")
Do While fname <> ""
bHadFiles = True
Set wBook = Workbooks.Open(filePath & fname, Format:=6, Delimiter:=",")
wBook.SaveAs filePath & Replace(fname, ".csv", ""), xlOpenXMLWorkbook
Application.CutCopyMode = False
wBook.Close False
fname = Dir()
Loop
If bHadFiles Then Kill filePath & "*.csv"
NextNo offense, but WTF is
colSF? It's a collection of folder names, right? Then just call it that. While we're at it, burn the hungarian notation. The name hadFiles already tells us thaCode Snippets
Sub Foo
' some code
Set colSF = GetSubFolders(CSVfolder)
For Each vFile In colSF
fname = Dir(CSVfolder & vFile & "\" & "*.csv")
Do While fname <> ""
bHadFiles = True
Application.ScreenUpdating = False
Set wBook = Workbooks.Open(CSVfolder & vFile & "\" & fname, Format:=6, Delimiter:=",")
wBook.SaveAs CSVfolder & vFile & "\" & Replace(fname, ".csv", ""), xlOpenXMLWorkbook
Application.CutCopyMode = False
wBook.Close False
fname = Dir()
Loop
If bHadFiles Then Kill CSVfolder & vFile & "\" & "*.csv"
Next
' more code
End SubPrivate Sub CommandButton1_Click()
Application.ScreenUpdating = False
Dim CSVfolder As String, CSVfolder1 As String, CSVfolder2 As String
CSVfolder = "C:\Charts\1\"
CSVfolder1 = "C:\Charts\2\"
CSVfolder2 = "C:\Charts\3\"
TransformFile CSVfolder
TransformFile CSVfolder1
TransformFile CSVfolder2
Application.ScreenUpdating = True
End Sub
Private Sub TransformFile(ByVal CSVfolder As String)
Dim fname As String
Dim vFile
Dim colSF As Collection
Dim wBook As Workbook
Set colSF = GetSubFolders(CSVfolder)
For Each vFile In colSF
fname = Dir(CSVfolder & vFile & "\" & "*.csv")
Do While fname <> ""
bHadFiles = True
Set wBook = Workbooks.Open(CSVfolder & vFile & "\" & fname, Format:=6, Delimiter:=",")
wBook.SaveAs CSVfolder & vFile & "\" & Replace(fname, ".csv", ""), xlOpenXMLWorkbook
Application.CutCopyMode = False
wBook.Close False
fname = Dir()
Loop
If bHadFiles Then Kill CSVfolder & vFile & "\" & "*.csv"
Next
End SubPrivate Sub CommandButton1_Click()
Application.ScreenUpdating = False
Dim folders As New Collection
folders.Add "1"
folders.Add "2"
folders.Add "3"
'must be a variant in order to loop over a string collection
'using a string array instead of a collection is another good option
Dim CSVFolder As Variant
For Each CSVFolder In folders
TransformFile CSVFolder
Next
Application.ScreenUpdating = True
End SubPrivate Sub TransformFile(ByVal CSVFolder As String)
Dim fname As String
Dim vFile
Dim colSF As Collection
Dim wBook As Workbook
Set colSF = GetSubFolders(CSVFolder)
For Each vFile In colSF
fname = Dir(CSVFolder & vFile & "\" & "*.csv")
Do While fname <> ""
bHadFiles = True
Application.ScreenUpdating = False
Set wBook = Workbooks.Open(CSVFolder & vFile & "\" & fname, Format:=6, Delimiter:=",")
wBook.SaveAs CSVFolder & vFile & "\" & Replace(fname, ".csv", ""), xlOpenXMLWorkbook
Application.CutCopyMode = False
wBook.Close False
fname = Dir()
Loop
If bHadFiles Then Kill CSVFolder & vFile & "\" & "*.csv"
Next
End SubCSVFolder & vFile & "\" & "*.csv"Context
StackExchange Code Review Q#94984, answer score: 7
Revisions (0)
No revisions yet.