Code review please

[mp_robinson_uk]

Hi All,

I would like to see if there is a better way of doing this.

The code does the following

I have a spreedsheet where on sheet one I have (number of columns can vary but this should not matter)

type start end title effect (first row is a header row)

set Mick Rob Mr yes

set Sam Rob Mr no

set John Lee Mr yes

set Dave Jac Mr 1

set Mick Kil Mr yes

set Bill Dav Mr 2

second page has just the start and end names

Mick Kil

John Lee

whenever the names match between the second and first page I would like the relivant row in the first page to have a green background i.e. match page2 column1 = page1 column2 and page2 column2 = page1 column3

Mick Kil matches row 6 so the row with "set Mick Kil Mr yes" would have a green background and

John Lee matches row 4 so the row with "set Mick Kil Mr yes" would have a green background

I did the following, it works but what could I have done better ;-)

Thanks for your help - great forum!

Option Explicit
 
Sub Add_Col_Row()
Dim LastRow1 As Long
Dim LastRow2 As Long
Dim Counter1 As Integer
Dim Counter2 As Integer
LastRow1 = Sheets("Sheet1").Cells.Find(What:="*", LookIn:=xlValues, _
SearchOrder:=xlByRows, SearchDirection:=xlPrevious).Row + 1
LastRow2 = Sheets("Sheet2").Cells.Find(What:="*", LookIn:=xlValues, _
SearchOrder:=xlByRows, SearchDirection:=xlPrevious).Row + 1
Counter1 = LastRow1 - 1
Counter2 = LastRow2 - 1
Do While Counter2 > 0
Do While Counter1 > 1
If Sheets("Sheet2").Cells(Counter2, 1).Value = _
Sheets("Sheet1").Cells(Counter1, 2).Value Then
If Sheets("Sheet2").Cells(Counter2, 2).Value = _
Sheets("Sheet1").Cells(Counter1, 3).Value Then
Sheets("Sheet1").Rows(Counter1 & ":" & Counter1).Select
With Selection.Interior
.ColorIndex = 35
.Pattern = xlSolid
End With
End If
End If
Counter1 = Counter1 - 1
Loop
Counter1 = LastRow1 - 1
Counter2 = Counter2 - 1
Loop
End Sub

[Zack Barresse]

... I did the following, it works but what could I have done better ;-)

I'd still suggest the Conditional Formatting.

.. http://www.vbaexpress.com/forum/showthread.php?t=1426

[TonyJollans]

I'd second that. Colour changes would then happen automatically whenever the data changed without running the code and checking everything which hadn't changed as well. Also your code assumes none of the cells are coloured to start with so can not be rerun unless you reset the colours first.

[mp_robinson_uk]

Hi All,

Thanks for your help.

Firefytr - when I tried your method I got an error message
- you may not use unions, intersections or array constants for Conditional Formating criteria. Whatever that means

TonyJollans - good points, but I know I will just be running this on data I have just imported in from a csv format and in this case the data will not change, if it does I will be importing fresh data from a new csv file.

Here is a simple example (signoff.zip), my macro is included and has been run so you will need to reset the colours.

Thanks for your help,

Mick

[TonyJollans]

Hi Mick,

Given what you say, your code will do the job.

To use conditional formatting, name your ranges in Sheet2 - don't name the whole columns as you have done, just the cells with data (in your example I changed mystart to $A1:$A3 and myend to $B1:$B3) - and do make sure the ranges are both the same size, then

Format > Conditional Formatting from the Menu
Select Formula Is under Conditipon 1
Enter =SUMPRODUCT(($B1=mystart)*($C1=myend))>0 in the Formula box
Choose your format and press OK



Note: I edited this post after reading your other thread and doing a bit more playing with the example. I had previously suggested a helper column to get round the CF restriction on referencing other Sheets but the named range does it so much better.

[mp_robinson_uk]

Hi Firefytr and TonyJollans,

I prefer your way though, it is much cleaner. I have changed the macro to use it.

Thanks Firefytr and TonyJollans for your help!

Thanks,

Mick

[mp_robinson_uk]

Hi TonyJollans,

Thanks I have now implemented using both the helper column and your new way, which I like the best. I'm glad because I can see how the helper column idea could be useful for other things I need to do.

I know I am not the best programmer by a long shot - I do a bit of work in langagues like perl but I just find this so different in how you go about doing things.

Thanks you very much for all your help.

Regards,

Mick