IF Else statement, to close files, in a Do Loop results in a continuous opening and closing of one file

Question

The following code gets stuck in a continuous loop opening and closing a file that does not satisfy the criteria of the If statement.

I hoped that by placing the Else statement just before Loop that it would continue the Loop and move down the Hotel List to the next file path. It reopens and closes the first file that doesn't satisfy the criteria of the If. Which in my instance is the third file on the list.

For those files not satisfying the If statement I want to close them and move on to the next. Apart from this issue, the code does as expected.

All of the code:

Sub Control_AR_AD_Copy_Hotel_DataArchive_And_Control()
'
' In summary loops through list of hotel file paths and copies the data archive and control information until complete.
'

' Creates list of hotels we expect an aged debtor report for.
'
    Sheets("Hotel_List").Select
    Range("A4:Z103").Select
    Selection.ClearContents
    Range("A1").Select
    Sheets("Tables").Select
    ActiveSheet.Range("$D$4:$Q$103").AutoFilter Field:=4, Criteria1:="Yes"
    Range("D4:Q103").Select
    Selection.Copy
    Sheets("Hotel_List").Select
    Range("A3").Select
    Selection.PasteSpecial Paste:=xlPasteValues, Operation:=xlNone, SkipBlanks:=False, Transpose:=False
    Application.CutCopyMode = False
    Cells.Select
    Cells.EntireColumn.AutoFit
    Cells.EntireRow.AutoFit
    Cells.EntireColumn.AutoFit
    Range("A1").Select

' Copy Data Archive to Previous and clear data archive
    Sheets("Data_Archive_All").Select
    Range("A2:Z2").Select
    Range(Selection, Selection.End(xlDown)).Select
    Selection.Copy
    Range("A2").Select
    Sheets("Data_Archive_Previous").Select
    Range("A2").Select
    Selection.PasteSpecial Paste:=xlPasteValues, Operation:=xlNone, SkipBlanks:=False, Transpose:=False
    Range("A2").Select
    Application.CutCopyMode = False
    Sheets("Data_Archive_All").Select
    Range("A2:Z2").Select
    Range(Selection, Selection.End(xlDown)).Select
    Selection.ClearContents
    Range("A2").Select
    Sheets("Control").Select
    Range("A1").Select
    
' Select cell where Loop begins as first line of data.
    Sheets("Hotel_List").Select
    Range("F4").Select

' Set Do loop to stop when an empty cell is reached.
    Do Until IsEmpty(ActiveCell)

' Open hotel file as read only.
    Dim my_wb As Workbook
    Dim file_path As String
    file_path = ActiveCell
    Set my_wb = Workbooks.Open(Filename:=file_path, ReadOnly:=True)

' Activate Workbook
    Dim hotel_wb As Workbook
    Set hotel_wb = ActiveWorkbook
    hotel_wb.Activate

' Set If Statement based upon values in Data_Archive and Control worrksheet
    If Sheets("Data_Archive").Range("G2") > "0" And Sheets("Control").Range("T18") = "Yes" Then
            
' Remove filter from Hotel's Data Archive and unprotect worksheet.
    Sheets("Data_Archive").Select
    Range("A2").Select
    ActiveSheet.Unprotect "DebRepAcc"
    ActiveSheet.AutoFilter.ShowAllData
    
' Apply filter and copy data from Hotel's Data Archive.
    Range("A2").Select
    ActiveSheet.Range("B1").AutoFilter Field:=25, Criteria1:="1"
    Range("$A$2:$Z$50001").Select
    Selection.Copy
    
' Paste data into Coonsolidated Data Archive in next blank cell.
    Workbooks("Consolidated_Accounts_Receivable_Aged_Debtors").Activate
    Sheets("Data_Archive_All").Select
    Range("A:A").Select
    Selection.Find(What:="", After:=ActiveCell, LookIn:=xlFormulas, LookAt _
        :=xlPart, SearchOrder:=xlByRows, SearchDirection:=xlNext, MatchCase:= _
        False, SearchFormat:=False).Activate
    ActiveCell.Select
    Selection.PasteSpecial Paste:=xlPasteValues, Operation:=xlNone, SkipBlanks:=False, Transpose:=False
    Range("A1").Select
    Application.CutCopyMode = False

' Remove filter from Hotel's Data Archive and protect worksheet.
    hotel_wb.Activate
    Sheets("Data_Archive").Select
    ActiveSheet.AutoFilter.ShowAllData
    ActiveSheet.Protect "DebRepAcc", DrawingObjects:=False, Contents:=True, Scenarios:=False, AllowFiltering:=True
            
' Copy data from Control worksheet.
    Sheets("Control").Select
    ActiveSheet.Calculate
    Range("C3:T18").Select
    Selection.Copy

' Paste Control Data into Control Convert worksheet.
    Workbooks("Consolidated_Accounts_Receivable_Aged_Debtors").Activate
    Sheets("Control_Convert").Select
    Range("A1").Select
    Selection.PasteSpecial Paste:=xlPasteValues, Operation:=xlNone, SkipBlanks:=False, Transpose:=False
    Range("A1").Select
    Application.CutCopyMode = False
    ActiveSheet.Calculate
    
' Copy data from Control_Convert into Control_Current in next blank cell.
    Sheets("Control_Convert").Select
    Range("A20:O20").Select
    Selection.Copy
    Sheets("Control_Current").Select
    Range("A:A").Select
    Selection.Find(What:="", After:=ActiveCell, LookIn:=xlFormulas, LookAt _
        :=xlPart, SearchOrder:=xlByRows, SearchDirection:=xlNext, MatchCase:= _
        False, SearchFormat:=False).Activate
    ActiveCell.Select
    Selection.PasteSpecial Paste:=xlPasteValues, Operation:=xlNone, SkipBlanks:=False, Transpose:=False
    Range("A1").Select
    Application.CutCopyMode = False
    
' Close hotel's workbook without saving changes.
    hotel_wb.Activate
    ActiveWorkbook.Close savechanges:=False

' Step down 1 row from present location.
    Workbooks("Consolidated_Accounts_Receivable_Aged_Debtors").Activate
    Sheets("Hotel_List").Select
    ActiveCell.Offset(1, 0).Select
    
' End If Function
    Else
    ActiveWorkbook.Close savechanges:=False
    End If

    Loop
   
'
' Copy data from Control_Current into Control_Archive in next blank cell.
'
    Workbooks("Consolidated_Accounts_Receivable_Aged_Debtors").Activate
    Sheets("Control_Current").Select
    Range("A2:O101").Select
    Selection.Copy
    Sheets("Control_Archive").Select
    Range("A:A").Select
    Selection.Find(What:="", After:=ActiveCell, LookIn:=xlFormulas, LookAt _
        :=xlPart, SearchOrder:=xlByRows, SearchDirection:=xlNext, MatchCase:= _
        False, SearchFormat:=False).Activate
    ActiveCell.Select
    Selection.PasteSpecial Paste:=xlPasteValues, Operation:=xlNone, SkipBlanks:=False, Transpose:=False
    Range("A1").Select
    Application.CutCopyMode = False
    Sheets("Control").Select
    Range("A1").Select
    
End Sub

Firstly you are using your workbook objects incorrectly. When you are working with a wb object, lets say `Set my_wb = Workbooks.Open(Filename:=file_path, ReadOnly:=True)` that will be how you interact with that particular workbook. In your case you open the workbook and store it in `my_wb` object, then add another workbook object `hotel_wb` which the same workbook object is stored into... When a workbook is open you can interact with it using that workbook object without using .Activate too. — data_sc, Jun 25 '22 at 20:38
First of all, your code looks primitive, no offence... Selecting and activating in your code context only consumes Excel resources, not bringing any benefit. Then, you use this code line `Set my_wb = Workbooks.Open(Filename:=file_path, ReadOnly:=True)` and immediately after, `Set hotel_wb = ActiveWorkbook`. What do you expect from the last code line? The Active workbook is anyhow `my_wb`. Opening a workbook, it becomes active... If you want referring to the previous workbook where you initially made a lot of selections, you should Set it at the beginning, before opening the other one... — FaneDuru, Jun 25 '22 at 20:44
Thank you for the comments above. No offence taken. Appreciate the advice. However, I don't think those answers solve my issue of the file not satisfying the IF statement continuously reopening and closing. I would expect the Loop to move on through the list as it does if I remove the If / Else Statement. — Andrew, Jun 25 '22 at 20:56
I guess your loop won't work because in the `Else` section you are not "stepping down 1 row from the present location" like you are doing at he "end" of the `If` section. That's probably what you are looking for. A part from that: your code is messy, you don't use variables efficiently and select way too much. Be thrilled because there is so much you can learn. ;) A good start might be [this one](https://stackoverflow.com/questions/10714251/how-to-avoid-using-select-in-excel-vba). — Evil Blue Monkey, Jun 25 '22 at 21:11
One thing (among many) `Sheets("Data_Archive").Range("G2") > "0"` is comparing to a string, which is not the same as a number. Try removing the quotes around `0`. Depends on your data, but if you do have numbers stored as strings on your sheet, fix that too — chris neilsen, Jun 25 '22 at 21:13
There are also good practice one might adopt like an overall structure of the code. Usually i start with the declarations of variables (the `Dim` this `Dim` that part), then the settings of as many variables as possible/practical, then the code, then the occasional debugging section. This way you can easily read and set all the variables you are using. You might also want to avoid any hardcoded data, especially if they are spread across the code. — Evil Blue Monkey, Jun 25 '22 at 21:17
@chris neilsen: how so? You have the `Do` opening, the declaration of the variables, the setting of the path based on the active cell and then the if statement which won't change the active cell if the conditions are not satisfied. Am i missing something? — Evil Blue Monkey, Jun 25 '22 at 21:20
@EvilBlueMonkey far call. Just one of the readability issues of that code mess — chris neilsen, Jun 25 '22 at 21:21
@chris neilsen: overall the code might be messy but in the end we just need to consider the case of a file that does not satisfy your condition. And in such case, there is only one action taken: the active workbook is closed. No other row can be selected because the part prior to the `If` does not change the active cell in any way. Therefore the loop will never end once it has encounter an unsatisfactory file. Do you agree? — Evil Blue Monkey, Jun 25 '22 at 21:30
@EvilBlueMonkey Thank you very much. Seems obvious now. I feel like Homer Simpson for not spotting that. lol. Thank you very much. Shifted the 'End If Function' to above the 'Step down 1 row...' and the code now does as required. Awesome. You're not evil, you're an angel. — Andrew, Jun 25 '22 at 21:32
@Andrew: thank you, glad to read that. Be careful that you still need to step the row down even in the case of a satisfactory file. Therefore you need in both section of the `If` the appropriate code to achieve said step. Since it must be taken in both cases, you might as well place said code outside of the `If-End If` statement (but of course before the closing the `Loop`). — Evil Blue Monkey, Jun 25 '22 at 21:36
@EvilBlueMonkey. You're welcome it's the least I can say or do. The advice you have given on a structure is great and sound advice. I will make a note of this and aim for better next time. It's been 20 years since I last tried to write any VBA. Half of what you see is copy and paste, and it probably shows! ;-) I better back in my 20s but not great. Programming isn't my bread and butter but comes in useful now and again when companies do not have adequate reporting systems in place and we pull information from around servers. Thanks again, you're a diamond geezer. — Andrew, Jun 25 '22 at 21:56
@Andrew: i work in a grocery shop and recently i've obtained a commercial pilot licence for helicopter (and hopefully i will be engaged in the aviation industry). I am not a professional coder. Even if i've created (unasked and unrewarded) a couple of Excel files (xlsm) used by small companies, it's more like an hobby that i've developed mostly by myself playing and searching for solutions online. Still i wouldn't say no to a good job in the code developement sector. I just don't have the status. Anyway: comments are supposed to be relevant to the question. We should not use it as a chat. ;) — Evil Blue Monkey, Jun 26 '22 at 09:13

IF Else statement, to close files, in a Do Loop results in a continuous opening and closing of one file

0 Answers0