6
\$\begingroup\$

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
\$\endgroup\$
6
\$\begingroup\$

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.

\$\endgroup\$
5
  • \$\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
    Jul 13 '12 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
    Jul 13 '12 at 18:08
  • \$\begingroup\$ That works beautifully and is much easier to read/look at! Thanks again. \$\endgroup\$
    – Gaffi
    Jul 13 '12 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
    Jul 13 '12 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
    Jul 13 '12 at 18:41
3
\$\begingroup\$

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!"
\$\endgroup\$
3
  • \$\begingroup\$ Yeah, that's not quite as simple, but still better than my original. Thanks! \$\endgroup\$
    – Gaffi
    Jul 14 '12 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 with Exit For. \$\endgroup\$
    – Daniel
    Jan 4 '13 at 18:23
  • \$\begingroup\$ True. However, mine has the luxury of not having to use an Exit call of any kind. \$\endgroup\$ Jan 5 '13 at 3:26

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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