PDA

View Full Version : Solved: Variable type question



Digita
07-12-2007, 12:43 AM
Hi everyone,

The following code is to merge all records from range A3 downwards from all sheets into sheet1. The data to be merged needs to be unique - ie if I have multiple records with string value of "SSSS" in column B in any sheet, only 1 entry is copied into sheet1. The problem I'm having is the code merges everything including duplicates.

The problem, I think, is the variable declaration. Local variable cl is defined a range and I'm trying to do a countif based on a range rather than a value of a range in the for loop. :banghead:



Sub Combine()
Dim cl As Range
Dim wks As Worksheet

For Each wks In ThisWorkbook.Worksheets
If wks.Name <> "Sheet1" And wks.Range("A3") <> "" Then
For Each cell In wks.Range(wks.Range("A3"), wks.Range("A5").End(xlDown))
If WorksheetFunction.CountIf(Worksheets("Master").Range("B:B"), cl) = 0 Then
cl.EntireRow.Copy Destination:=Worksheets("Sheet1").Range("A65536").End(xlUp).Offset(1, 0)
End If
Next cl
End If
Next wks

End Sub


Any suggestion please. Thanks for your help.

Regards


KP

johnske
07-12-2007, 12:56 AM
It will error because you haven't set the range, e.g.


Set cl = Range("A1:A10")

xld
07-12-2007, 01:10 AM
I think cl should be cell. No Option Explicit!

Norie
07-12-2007, 09:46 AM
The problem has nothing to do with variable declaration as far as I can see.

If you only want unique values why are you testing if the count is 0?

Surely you should be checking for a count of 1?

Digita
07-12-2007, 05:05 PM
Hi everyone,

XLD has quickly identified a typo which prevented the code from running. I declared Cl as range but I wrongly refer this variable as Cell in the For loop. The codes works perfectly without expression Option Explicit.

Johnske, your suggestion is noted & thanks. However, the expression wks.Range(wks.Range("A3"), wks.Range("A3").End(xlDown)) is more dynamic in terms of variable ranges in sheets.

Norie, you're right - nothing wrong with the variable object being declared as range. I use the countif is to copy only unique entries to sheet1. In other words, when an entry has not been copied to sheet1 before, the countif function returns 0 and the code proceeds to copying.

The working code is as follows:




Sub Combine()
Dim cl As Range
Dim wks As Worksheet


For Each wks In ThisWorkbook.Worksheets
If wks.Name <> "Sheet1" And wks.Range("A3") <> "" Then
For Each cell In wks.Range(wks.Range("A3"), wks.Range("A3").End(xlDown))
If WorksheetFunction.CountIf(Worksheets("Master").Range("B:B"), cl) = 0 Then
cl.EntireRow.Copy Destination:=Worksheets("Sheet1").Range("A65536").End(xlUp).Offset(1, 0)
End If
Next cl
End If
Next wks


End Sub



Your inputs are greatly appreciated. Have a nice weekend.


Best regards



KP

johnske
07-12-2007, 06:56 PM
... The codes works perfectly without expression Option Explicit...
:bug: you miss the point entirely - if you had used Option Explicit, Visual Basic would have told you that you had used an undeclared variable. In fact, if you type Option Explicit at the top of a code module and paste your code (above) beneath that, then click 'Debug' > 'Compile VBAProject', it will still tell you that...

You need Option Explicit to force you to make your code logically consistent, otherwise Visual Basic is more or less free to interpret your code in its own way. This may possibly - or may not - give you the intended result for now, but this is just by chance, it can give you entirely different results when used in another workbook with a different layout :)

Digita
07-12-2007, 11:30 PM
I have added the Option explicit on top of the code.

Thanks again Johnske for your explanation.

Regards


KP