The below code is real and in use, but I've modified it to simplify the process/make it easier to explain.
The purpose of this code is to combine data from multiple data sources. All sources are .xls files, but the names will vary based on the date of creation, and the formatting can vary based on the user.
Because the names are variable, I have opted to prompt the
user to select each of the necessary files, rather than try to code in an auto-selection.
Also, since the end-users like to have things as simple as possible, I've condensed this
process into one function (there are multiple extra functions, all wrapped in this main
function) as opposed to having to manually begin multiple processing steps. Since there
are a number of files involved, I have designed this to have the user pre-open each
of the necessary files (which all live in different locations on a network) before
beginning this process. As a result, I have multiple If
checks to verify that a user
has not cancelled mid-way (say, they forgot to open one of the files earlier and didn't
realize until they were prompted for it).
Is there a better way to collect all the files that are needed for processing while allowing the user to cancel at any step in the process?
There is nothing wrong for me to have such deep nesting, but I feel like this may be bad
form. I believe I can put each check all on one line with And
, but that I think would
also become unwieldy/harder to debug if there was an issue.
I have added comments to the code to explain what may be missing from the example, but feel free to ask if I've missed anything.
Update: I've made it a little nicer by putting all the workbook/sheet assignments after the last if.
Option Explicit
Sub Example()
Dim Book0 As Workbook
Dim Sheet0 As Worksheet
Dim Book1 As Workbook
Dim Book2 As Workbook
Dim Sheet2 As Worksheet
Dim Book3 As Workbook
Dim Sheet3 As Worksheet
Dim Book4 As Workbook
Dim Sheet4 As Worksheet
Dim Book5 As Workbook
Dim Sheet5 As Worksheet
If MsgBox("Before beginning, please make sure you have each of the following files open:" & vbCr & vbCr & vbTab & _
"Book1" & vbCr & vbTab & "Book2" & vbCr & vbTab & _
"Book3" & vbCr & vbTab & "Book4" & vbCr & vbTab & _
"Book5" & vbCr & vbTab & "Book6" & vbCr & vbCr & _
"If these files are ready, then please click 'OK' (or 'Cancel' to cancel and try again).", _
vbOKCancel, "All files ready?") = vbOK Then
' sListing is a 2d array of strings, holding the names of workbooks/sheets.
' sListing(0, 0) is the name of the first workbook in a selection.
' sListing(0, 1) is the name of the (optionally selected) worksheet within sListing(0, 0).
' sListing(1, 0) is the name of the second workbook in a selection. (And so on...)
' Reset_sListing assigns all values within the array to a null string.
Reset_sListing
' GetBook is a function that finds all open workbooks within an instance of Excel.
' The function displays a list of all workbooks on a form and prompts the user to select one.
' If only one string argument is passed, then is searches only for a workbook.
' The name of the selected workbook is then added to sListing at position (n, 0),
' where n is the number passed as the third argument to GetBook. If two string arguments
' are passed, then the function will do the same as above, but for a sheet within the
' selected workbook, passing the value to sListing at position (n, 1). The function will
' return true if the user made a proper selection on the form(s), false if there was
' an error or the user cancels.
If GetBook("Please select the Book0.", "Please select a sheet within Book0.", 0) Then
If GetBook("Please select the Book1.", , 1) Then
If GetBook("Please select the Book2.", "Please select a sheet within Book2.", 2) Then
If GetBook("Please select the Book3.", "Please select a sheet within Book3.", 3) Then
If GetBook("Please select the Book4.", "Please select a sheet within Book4.", 4) Then
If GetBook("Please select the Book5.", "Please select a sheet within Book5.", 5) Then
If GetBook("Please select the Book6.", "Please select a sheet within Book6.", 6) Then
Set Book0 = Workbooks(sListing(0, 0))
Set Sheet0 = Book0.Sheets(sListing(0, 1))
Set Book1 = Workbooks(sListing(1, 0))
Set Book2 = Workbooks(sListing(2, 0))
Set Sheet2 = Book2.Sheets(sListing(2, 1))
Set Book3 = Workbooks(sListing(3, 0))
Set Sheet3 = Book3.Sheets(sListing(3, 1))
Set Book4 = Workbooks(sListing(4, 0))
Set Sheet4 = Book4.Sheets(sListing(4, 1))
Set Book5 = Workbooks(sListing(5, 0))
Set Sheet5 = Book5.Sheets(sListing(5, 1))
Set Book6 = Workbooks(sListing(6, 0))
Set Sheet6 = Book6.Sheets(sListing(6, 1))
'Do stuff with books/sheets.
MsgBox "Process complete!"
End If
End If
End If
End If
End If
End If
End If
End If
End Sub
2 Answers 2
You can replace all of the Getbook nested ifs with the following. Place this first:
Dim i As Long
For i = 0 To 6
If Not Getbook("Please select the Book" & i & ".", "Please select a sheet within Book" & i & ".", i) Then
Exit Sub
End If
Next i
Then below that list what you want to happen:
Set Book0 = Workbooks(sListing(0, 0))
Set Sheet0 = Book0.Sheets(sListing(0, 1))
Set Book1 = Workbooks(sListing(1, 0))
Set Book2 = Workbooks(sListing(2, 0))
Set Sheet2 = Book2.Sheets(sListing(2, 1))
Set Book3 = Workbooks(sListing(3, 0))
Set Sheet3 = Book3.Sheets(sListing(3, 1))
Set Book4 = Workbooks(sListing(4, 0))
Set Sheet4 = Book4.Sheets(sListing(4, 1))
Set Book5 = Workbooks(sListing(5, 0))
Set Sheet5 = Book5.Sheets(sListing(5, 1))
Set Book6 = Workbooks(sListing(6, 0))
Set Sheet6 = Book6.Sheets(sListing(6, 1))
'Do stuff with books/sheets.
MsgBox "Process complete!"
This replaces the huge amount of ifs with a loop that has the same result. I.E. Operation continues only if every if statement returns true, and if any return false the sub exits.
Edit: It should also be possible to standardize your Set Book0 Set Sheet0 sections as well by using arrays.
To do that you would do something like this...:
Dim workbooks(6) As Workbook
Dim sheets(6) As Worksheet
Dim i As Long
For i = 0 To 6
If Not Getbook("Please select the Book" & i & ".", "Please select a sheet within Book" & i & ".", i) Then
Exit Sub
Else
Set workbooks(i) = workbooks(slisting(i, 0))
Set sheets(i) = workbooks(i).sheets(slisting(i, 1))
End If
Next i
'Do stuff with books/sheets.
MsgBox "Process complete!"
This way once the loop is done all the workbooks have been assigned to the array.
-
\$\begingroup\$ I like this idea a lot. (kicking myself for not doing it already...) When I started reading, I was thinking 'no, that's not going to work' because the specific prompt for the workbooks will vary by workbook. However, you're absolutely right that I can use arrays. I can also use arrays to hold the prompt strings! I will try an implement and see what I come up with. Thanks! \$\endgroup\$Gaffi– Gaffi2012年07月13日 17:59:18 +00:00Commented Jul 13, 2012 at 17:59
-
\$\begingroup\$ And for the record, VBA assigns arrays with a 1 base, instead of 0, so those have to be
sheets(0 to 6) As Worksheet
. \$\endgroup\$Gaffi– Gaffi2012年07月13日 18:08:55 +00:00Commented Jul 13, 2012 at 18:08 -
\$\begingroup\$ That works beautifully and is much easier to read/look at! Thanks again. \$\endgroup\$Gaffi– Gaffi2012年07月13日 18:19:54 +00:00Commented Jul 13, 2012 at 18:19
-
\$\begingroup\$ Check your locals window. When I created the arrays, they were 0 based. This is the default behavior. (msdn.microsoft.com/en-us/library/aa266179(v=vs.60).aspx) But if you want to be certain you can indeed specify. It would probably be a good practice. \$\endgroup\$Daniel– Daniel2012年07月13日 18:38:15 +00:00Commented Jul 13, 2012 at 18:38
-
\$\begingroup\$ You are right. I think I was getting confused with VBA
array(n)
is 0 to n (n+1 total items), whereas in many others,array(n)
is 0 to n-1 (n total items). Either way, this was extremely helpful. \$\endgroup\$Gaffi– Gaffi2012年07月13日 18:41:49 +00:00Commented Jul 13, 2012 at 18:41
In your case, I think @DanielCook's example of using a for-loop is your best option. But I wanted to share a trick I use when you can't really viably use a for-loop. In this example, if any of the calls to GetBook()
return False
then Result
will be False
. As I said though, this isn't as good as a for-loop, but when your conditions are something that can't easily be iterated, this works well.
Dim Result As Boolean: Result = True
Result = Result And GetBook("Please select the Book0.", "Please select a sheet within Book0.", 0)
Result = Result And GetBook("Please select the Book1.", , 1)
Result = Result And GetBook("Please select the Book2.", "Please select a sheet within Book2.", 2)
Result = Result And GetBook("Please select the Book3.", "Please select a sheet within Book3.", 3)
Result = Result And GetBook("Please select the Book4.", "Please select a sheet within Book4.", 4)
Result = Result And GetBook("Please select the Book5.", "Please select a sheet within Book5.", 5)
Result = Result And GetBook("Please select the Book6.", "Please select a sheet within Book6.", 6)
If Result Then
Set Book0 = Workbooks(sListing(0, 0))
Set Sheet0 = Book0.Sheets(sListing(0, 1))
Set Book1 = Workbooks(sListing(1, 0))
Set Book2 = Workbooks(sListing(2, 0))
Set Sheet2 = Book2.Sheets(sListing(2, 1))
Set Book3 = Workbooks(sListing(3, 0))
Set Sheet3 = Book3.Sheets(sListing(3, 1))
Set Book4 = Workbooks(sListing(4, 0))
Set Sheet4 = Book4.Sheets(sListing(4, 1))
Set Book5 = Workbooks(sListing(5, 0))
Set Sheet5 = Book5.Sheets(sListing(5, 1))
Set Book6 = Workbooks(sListing(6, 0))
Set Sheet6 = Book6.Sheets(sListing(6, 1))
'Do stuff with books/sheets.
MsgBox "Process complete!"
End If
EDIT
I realized that in @DanielCook's example, if you want the "Process complete!" message box to show up regardless of whether GetBook()
ever returns False
then a combination of his example and mine would be better..
Dim Workbooks(6) As Workbook
Dim Sheets(6) As Worksheet
Dim Result As Boolean: Result = True
Dim i As Long
For i = 0 to 6
Result = Result And GetBook("Please select the Book" & i & ".", "Please select a sheet within Book" & i & ".", 1)
If Result Then
Set Workbooks(i) = Workbooks(sListing(i,0))
Set Sheets(i) = Workbooks(i).sheets(sListing(i, 1))
End If
Next
' Some code you want to run regardless of the result
MsgBox "Process complete!"
-
\$\begingroup\$ Yeah, that's not quite as simple, but still better than my original. Thanks! \$\endgroup\$Gaffi– Gaffi2012年07月14日 15:22:56 +00:00Commented Jul 14, 2012 at 15:22
-
\$\begingroup\$ This is a bit late of a comment, but in response to your edit, you could also achieve the same results by replacing my line that says
Exit Sub
withExit For
. \$\endgroup\$Daniel– Daniel2013年01月04日 18:23:06 +00:00Commented Jan 4, 2013 at 18:23 -
\$\begingroup\$ True. However, mine has the luxury of not having to use an
Exit
call of any kind. \$\endgroup\$Drew Chapin– Drew Chapin2013年01月05日 03:26:52 +00:00Commented Jan 5, 2013 at 3:26