current community

your communities

more stack exchange communities

Stack Exchange Inbox Reputation and Badges
tour help
Sign up ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 2 down vote favorite

I would like to speed up this piece of code. The macro gets a search variable from the worksheet "RAW_DATA_ARCHIVE" and then it should find it in the worksheet "RAW DATA". When it finds accordance then the value of column A from worksheet "RAW DATA" will be written in the last column of "RAW_DATA_ARCHIVE".

 For i = 2 To ws1LRow
    SearchString = ws1.Range("A" & i).Value
    Set aCell = ws2.Columns(5).Find(What:=SearchString, LookIn:=xlValues, _
                LookAt:=xlWhole, SearchOrder:=xlByRows, SearchDirection:=xlNext, _
                MatchCase:=False, SearchFormat:=False)
    'If match found
    If Not aCell Is Nothing Then
        ws1.Cells(i, 80).Value = ws2.Cells(aCell.Row, 1).Value

    End If
Next
share|improve this question
1  
You don't need a VBA macro to do this. Have you seen what VLOOKUP can do for you? Try something like this in column 80 of sheet Raw_Data_Archive: =IFERROR(VLOOKUP(A2,'RAW DATA'!E:E,1,FALSE),"") –  Mat's Mug♦ Apr 10 at 13:30

2 Answers 2

up vote 3 down vote

If I understand correctly what you're doing, then VBA is simply not the right tool for this task. Excel has a built-in VLOOKUP function specifically for this purpose - try something like this in column 80 of the RAW_DATA_ARCHIVE sheet:

=IFERROR(VLOOKUP(A2,'RAW DATA'!E:E,1,FALSE),"")

That said, you did post some VBA code, albeit just a loop. Here are some thoughts, mostly on naming style:

  • ws1 and ws2 would probably be better off as sourceSheet and targetSheet.
  • ws1LRow would be more meaningful as simply lastRow
  • i is usually fine as a loop counter, but here it has a clear meaning: targetRow
  • aCell is really a searchResult

Using such meaningful names instantly makes it easier to read the code, which makes it easier to maintain/modify without breaking anything.

It also eliminates the need for explanatory comments:

'If match found
If Not aCell Is Nothing Then

vs.

If Not searchResult Is Nothing Then

A number of things explain why your code is slow.

  1. You're using procedural code instead of built-in worksheet functions
  2. You're writing to cells in a loop, presumably with Application.ScreenUpdating = True and Application.Calculation = xlCalculationAutomatic, which means many things happen every time you write a value into a cell.
share|improve this answer
    
Firstly the target sheet get replaced every week, so i need vba. Besides the "vloockup" doesn't help me, because my target value is on the left side of the search value. –  NewOne Apr 14 at 10:17
    
Had you answered to my comment 4 days ago my answer might have been different. I see you're using index+match here.... well, use that instead of a vlookup and you'll save yourself 10 minutes of looping.WorksheetFunction is calling, well, the worksheet functions after all - only much, much slower. –  Mat's Mug♦ Apr 14 at 11:39
up vote 0 down vote

To solve this problem I found the "Index-Match" functions. It is a little bit faster but also take over 10 minutes for about 160000 Cells. Meanwhile, I think it's too much data to get it faster.

With Application.WorksheetFunction
    On Error Resume Next
    For Each searchCell In searchRange
        targetSheet.Cells(searchCell.Row, 80) = .Index(sourceSheet.Columns(1), .Match(searchCell.Value, sourceSheet.Columns(2), 0))
    Next
End With
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.