PDA

View Full Version : [SOLVED] For Each Loop - Strange Behaviour (logic breakdown)



vanhunk
11-25-2013, 05:08 AM
For Each Loop - Strange BehaviourI have a list of names and use the FOR EACH Loop to check if a worksheet exists in the workbook with the same. It works perfectly up to the point where such a worksheet actually exists, it indicates that this worksheet does indeed exist. However, after this point it erroneously indicates the existence of worksheets with the remainder of the names in the list.For example I have a list with the following names: "Name1", "Name2", "Name3", "Name4", "Name5", "Name6", and "Name7". The workbook contains sheets with the names "Name4" and "Name7". The code is suppose to indicate whether sheets with names , "Name1", "Name2", "Name3", "Name4", "Name5", "Name6", and "Name7" exist in the workbook. It does exactly that untill it encounters the name "Name4" in the list. It correctly indicates that sheet "Name4" does indeed exist, but then continue indicating that the sheets "Name5" and "Name6" also exist, which in this case does not.I would appreciate an understanding of this behaviour and how the code should be changed to achieve the intended results.Note: Should the code end up in one line I would appreciate some assistance in correcting it, I have tried everything and every time it ends up in one long line (holding thumbs)!Regards,vanhunk

Sub VerifyCurrentList()
'Verify that the proposed sheet name does not already exist in the workbook:'------------------------------------------------------------------------------
Dim strContractor As String
Dim wks As Worksheet
Dim bln As Boolean
Dim oRange As Range
Dim rCell As Range
'Define the search range:
Set ws = Worksheets("Start Sheet")
Set oRange = ws.Range(Range("ContractorsList").Offset(1, 0), Range("ContractorsList").Offset(30, 0).End(xlUp))
For Each rCell In oRange
strContractor = rCell.Value
'MsgBox strContractor
On Error Resume Next
Set wks = ActiveWorkbook.Worksheets(strContractor)
On Error Resume Next
If Not wks Is Nothing Then
bln = True Else
bln = False
Err.Clear
End If
If bln = False Then
MsgBox strContractor & " is not current!"
Else
MsgBox strContractor & " is current!"
End If
Next rCell
End Sub

snb
11-25-2013, 05:30 AM
Sub M_snb()
sn=sheets("Start Sheet").range("ContractorsList").Offset(1, 0).specialcells(2)

for j=1 to ubound(sn)
msgbox sn(j,1) & "does" & iif(evaluate("Isref(" & sn(j,1) & "!A1)"),""," not") & " exist"
Next
End Sub

vanhunk
11-25-2013, 05:59 AM
Sub M_snb()
sn=sheets("Start Sheet").range("ContractorsList").Offset(1, 0).specialcells(2)
for j=1 to ubound(sn)
msgbox sn(j,1) & "does" & iif(evaluate("Isref(" & sn(j,1) & "!A1)"),""," not") & " exist"
Next
End Sub
Hi snb I get "Run time error 13: Type mismatch in the msgbox line. Thanks, vanhunk

snb
11-25-2013, 07:28 AM
I don't 'get it'. ;)


You can check stepping through the code: F8

Aflatoon
11-25-2013, 07:51 AM
To do it the original way, you must set wks = Nothing after you have located it:
If Not wks Is Nothing Then
bln = True
Set wks = Nothing
Else

'...

Paul_Hossler
11-25-2013, 07:56 AM
I don't think your two 'On Error Resume Next' statements are correct





Option Explicit ' <<<< added
Sub VerifyCurrentList()
Dim strContractor As String
Dim wksStart As Worksheet, wksContractor As Worksheet ' <<<< added wksContractor
Dim blnContractorSheetExists As Boolean
Dim oRange As Range
Dim rCell As Range

'Define the search range:
Set wksStart = Worksheets("Start Sheet")

Set oRange = wksStart.Range(Range("ContractorsList").Offset(1, 0), Range("ContractorsList").Offset(30, 0).End(xlUp))

For Each rCell In oRange
strContractor = rCell.Value

'MsgBox strContractor
Set wksContractor = Nothing ' <<<< I'd use a On Error differently
On Error Resume Next
Set wksContractor = ActiveWorkbook.Worksheets(strContractor)
On Error Goto 0

blnContractorSheetExists = Not (wksContractor Is Nothing)

If blnContractorSheetExists Then
MsgBox strContractor & " is current!"
Else
MsgBox strContractor & " is not current!"
End If
Next rCell
End Sub





Paul

vanhunk
11-25-2013, 08:01 AM
@Aflatoon Thank you very much, it is what I was looking for. Why is it necessary to do that, I would have thought that
Set wks = ActiveWorkbook.Worksheets(strContractor) would have taken care of it? By the way you don't by any change have a solution to me problem in terms of the one-liner? Thanks again, vanhunk

vanhunk
11-25-2013, 08:06 AM
@Paul Thank you Paul the code is doing exactly what it is suppose to be doing now. What do you mean by "I'd use an On Error differently"? Do you refer to the change you made, i.e. changing the second "On Error Resume Next" to "On Error Goto 0", or did you have something else in mind? Thanks. Regards, vanhunk

Paul_Hossler
11-25-2013, 11:58 AM
Glad it's working

I think the 2 Resume Next's were unneeded (probably didn't hurt), but the Goto 0 was in the wrong place

Mostly matter of style and personal opinion:

I think that the one statement that you really need to catch an error on is the second Set


1. I tend to be very wordy so I like to explicitly Set a condition I can test for (Set ... = Nothing) first

2. Tell Excel to ignore ALL errors (On Error Resume Next)

3. Execute the statement that could cause an error (Set wksContractor ...)

4. Tell Excel to stop ignoring errors (On Error Goto 0)

5. Then test to see if the condition in #1 is still there ( Is Nothing)


In general (i.e. most of the time) I bracket the potential error-generating statement: On Error Resume Next right before, and On Error Goto 0 right after.

You should never just let Excel go blindly along ignoring errors for longer than absolutely necessary




Set wksContractor = Nothing ' <<<< I'd use a On Error differently
On Error Resume Next
Set wksContractor = ActiveWorkbook.Worksheets(strContractor)
On Error Goto 0
blnContractorSheetExists = Not (wksContractor Is Nothing)



There are more sophisticated and powerful (and complicated) ways to handle errors, but for relatively simple things, I prefer to do it the way above. All the code that could cause an error and what to do if there is one, is in one place

Further un-asked for but volunteered anyway suggestion: You will save yourself a lot of head scratching if you used more descriptive variable names, i.e. blnContractorSheetExists and not just bln. The computer doesn't care, but since you'll have to read the macro later I think you're find it much easier to trace through the logic after 6 months

Paul

vanhunk
11-25-2013, 11:55 PM
@Paul Hossler Hi Paul, I like and appreciate your response, have a great day. Regards vanhunk