Skip to main content
Code Review

Return to Answer

Commonmark migration
Source Link

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.

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.

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.

Source Link
RubberDuck
  • 31.2k
  • 6
  • 74
  • 176

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.

lang-vb

AltStyle によって変換されたページ (->オリジナル) /