Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I have col D (in sheet1 called Students), col A (in Sheet2 called Students too) and col B (in sheet2 called Age). In colD there are a lot of similar Students names that are sorted from A to Z.

I have this code that finds matched students name and put his age in col D (sheet1):

Set rngSearch = Sheets("Sheet1").Range("D:D")
For Each rngSearch In Drng
    Set rngFound = Arng.Find(What:=rngSearch, LookIn:=xlValues, LookAt:=xlWhole,   SearchOrder:=xlByRows, SearchDirection:=xlNext, MatchCase:=False, SearchFormat:=False)
    'if studentname is found:
     If Not rngFound Is Nothing Then
         rngSearch.Offset(0, 0) = rngFound.Offset(0, 1)
     Else
         rngSearch.Offset(0, 0) = "-"  
     End If
Next

I want to loop col D once and if exists a match students name in colA (sheet2), I want to copy in colD (sheet1) the B column (sheet2) that matches to colA (sheet2).

I sorted colD because I don't want to loop every similar students name. I mean: if a student name (for example: Andrew) in colD matches to a student name ('Andrew') in colA, then copy colB (matches to col A- for example Andrew has 15 years old) in colD (sheet1). And if the string 'Andrew' is repeated in colD (I found it in another cells), don't loop for it again colB (sheet2), but copy the value from the first string.

For example:

(Sheet1)

colD: Students:    
row1: Andrew         
row2: Andrew         
row3: Andrew         
row4: Andrew         
row5: Andrew         
row6: Andrew         
row7: Ben         
row8: Ben         
row9: Edoardo         
row10: Helen         
row11: Leonardo         
row12: Leonardo         
row13: Robert         
rowN: ..

(Sheet2)

      Students:  colB: Age:       
row1: Michael          16         
row2: Timoth           20         
row3: Andrew           15         
row4: Edoardo          19         
row5: Ben              13         
row6: Robert           24         
row7: Helen            17

What I want:

(Sheet1)

colD: Students:        
row1: 15         
row2: 15         
row3: 15         
row4: 15         
row5: 15        
row6: 15         
row7: 13         
row8: 13         
row9: 19         
row10: 17         
row11: 0         
row12: 0         
row13: 24         
rowN: ..

Can anyone help me to optimize this code, please?

share|improve this question
    
I rolled back your last edit. There's a text box and an orange "answer" button at the bottom of the page. Please be sure to include a description of how you improved the code as well. Code only answers are frowned upon. –  RubberDuck Aug 19 at 11:26

2 Answers 2

up vote 2 down vote accepted

As in a lot of cases, you don't need to do what you're trying to accomplish. The best thing to do here is simply use one of the built in formulas. In this case, you want VLOOKUP. Using VLOOKUP, there's no need to sort the sheet.

VLOOKUP(sheet1!$A1,sheet2!A:B,2,FALSE)

But this is Code Review, so one is in order.


I'm assuming this is a typo.

Set rngSearch = Sheets("Sheet1").Range("D:D")
For Each rngSearch In Drng

And what you really mean is:

Drng = Sheets("Sheet1").Range("D:D")
For each rngSearch In Drng

Which makes me want to mention that I really hate Hungarian notation. I like this mixed and reversed notation even less. The IDE will tell you that it's a Range, but if you really just need your code to spell it out, just spell it out. At least be consistent.

rngSearch >> searchRange

Drng >> rngD >> columnD


If your subroutine needs the sheet to be sorted, then your subroutine should sort sort it itself, but I recommend against sorting the sheet at all unless your user expects the sheet to be sorted a certain way. Messing with the sort order unexpectly makes for a poor user experience. Albeit not as poor of an experience as your code producing unexpected results.


This line scrolls off the screen. You should try to keep everything on the screen if possible.

Set rngFound = Arng.Find(What:=rngSearch, LookIn:=xlValues, LookAt:=xlWhole,   SearchOrder:=xlByRows, SearchDirection:=xlNext, MatchCase:=False, SearchFormat:=False)

You've got two options to help solve this. You can either skip the verbose method of using argument identifiers and simply not pass the missing argument into Find,

Set rngFound = Arng.Find(rngSearch, , xlValues, xlWhole, xlByRows, xlNext, False, False)

Or use line continuations.

Set rngFound = Arng.Find(What:=rngSearch, _
                         LookIn:=xlValues, _
                         LookAt:=xlWhole, _  
                         SearchOrder:=xlByRows, _
                         SearchDirection:=xlNext, _
                         MatchCase:=False, _
                         SearchFormat:=False)

There's a small optimization to be had by only searching column D until there are no more values. Find the last used cell in the sheet and only search until you reach that cell. As it is, I think your code does many more iterations than it has to.

share|improve this answer
    
You're right, but I solved my problem yet. However, thanks for the feedback! –  musiclandia Aug 19 at 11:09
    
You're welcome. Welcome to Code Review by the way. –  RubberDuck Aug 19 at 11:10
    
Also, if you solved your issue, consider posting an answer to your question. –  RubberDuck Aug 19 at 11:11
1  
Thank you. I really appreciate the site! I post the code below. –  musiclandia Aug 19 at 11:21

I solved the problem. Here is the code:

Set rngSearch = Sheets("Sheet1").Range("D:D")
For Each rngSearch In Drng
    'all activities are searched in Arng:
    Set rFnd = Sheets("Sheet1").Range("D2:D" & lastRowD).Find(What:=sText, LookAt:=xlPart)
    Set rngFound = Arng.Find(What:=rngSearch, LookIn:=xlValues, LookAt:=xlWhole, SearchOrder:=xlByRows, SearchDirection:=xlNext, MatchCase:=False, SearchFormat:=False)

    If Not rFnd Is Nothing Then
        rngSearch.Offset(0, 0) = rngSearch.Offset(-1, 0) 'if found multiple strings, don't loop column A but copy the row above
    Else
        'if activity is found:
        If Not rngFound Is Nothing Then
            rngSearch.Offset(0, 0) = rngFound.Offset(0, 1)
        Else
            rngSearch.Offset(0, 0) = "0"
        End If
    End If
Next
share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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