Search for string in many WBs and post results into another WB I want to search for a string inserted in TextBox1 on Mainwb within a range of data located in Workbook officerA Worksheet DATA!(A2,G2000), and then paste results found into Workbook Mainwb.sheet("MAIN SCREEN").Range (A5,G500)

I am totally new to VBA and wrote this code quoting from many sources. I'd like to know how it can be improved.

This is used to search within the same workbook:

Sub CommandButton1_Click()
Dim wb1 As Workbook, Wb2 As Workbook
Dim ws1 As Worksheet, ws2 As Worksheet
Dim strSearch As String
Dim f As Variant
Dim fAddress As String
Dim fRow As Long
Dim cellA As Variant
Dim cellB As Variant


 Set Wb2 = Workbooks.Open("C:\Users\elhayani\Desktop\development\AML db\OfficerA.xlsx") 
  Set wb1 = Workbooks.Open("C:\Users\elhayani\Desktop\development\AML db\Mainwb.xlsm")
   Set ws2 = wb1.Sheets("MAIN SCREEN").Range("A5:G2000")
   Set ws1 = Wb2.Worksheets("DATA")  

strSearch = TextBox1.Value

ws1.Range("A5:G2000").ClearContents
Set dmr = Workbooks.Open("C:\Users\aselhayani\Desktop\Excel Reports\OfficerA.xlsx")
Set dmr = Worksheets("DATA")
strSearch = InputBox("Please enter T24 ID:", "Search Value")

pasteRowIndex = 5

If strSearch = vbNullString Then
    MsgBox ("User canceled, or did not enter a value.")
    Exit Sub
End If

With ws1.Range("A2:G2000")

    Set f = .Find(strSearch, LookIn:=xlValues)
    If Not f Is Nothing Then
        fAddress = f.Address
        Do
            fRow = f.Row
            cellA = ws2.Cells(fRow, 1).Value
            cellB = ws2.Cells(fRow, 2).Value
            cellC = ws2.Cells(fRow, 3).Value
            cellD = ws2.Cells(fRow, 4).Value
            cellE = ws2.Cells(fRow, 5).Value
            cellF = ws2.Cells(fRow, 6).Value
            cellG = ws2.Cells(fRow, 7).Value

            ws1.Cells(pasteRowIndex, 1) = cellA
            ws1.Cells(pasteRowIndex, 2) = cellB
            ws1.Cells(pasteRowIndex, 3) = cellC
            ws1.Cells(pasteRowIndex, 4) = cellD
            ws1.Cells(pasteRowIndex, 5) = cellE
            ws1.Cells(pasteRowIndex, 6) = cellF
            ws1.Cells(pasteRowIndex, 7) = cellG

            pasteRowIndex = pasteRowIndex + 1

            Set f = .FindNext(f)

        Loop While Not f Is Nothing And f.Address &lt;&gt; fAddress
    End If
End With
MsgBox "Search Done"
End Sub
Variables

Good job is making sure to declare all variables with a type:

 Dim wb1 As Workbook, Wb2 As Workbook


However, there's no real reason to group your variables like that; as you see with your strings it would get too long.

More on variables - give them some meaningful names. And avoid that hungarian naming:

Dim sourceWorkbook As Workbook
Dim targetWorkbook As Workbook
Dim sourceSheet As Worksheet
Dim targetSheet As Worksheet
Dim searchValue As String
Dim foundAddress As String
Dim foundRow As Long
Dim searchArray As Variant


Oh no, you didn't declare these variables:

  cellA = ws2.Cells(fRow, 1).Value
  cellB = ws2.Cells(fRow, 2).Value
  cellC = ws2.Cells(fRow, 3).Value
  cellD = ws2.Cells(fRow, 4).Value
  cellE = ws2.Cells(fRow, 5).Value
  cellF = ws2.Cells(fRow, 6).Value
  cellG = ws2.Cells(fRow, 7).Value


Why not use a range or an array? Seems like it will always be columns 1:7. Something like

For Each f In searchRange 'or your specific range
    Set f = searchRange.Find(searchValue, LookIn:=xlValues)
    If Not f Is Nothing And f.Address &lt;&gt; fAddress Then
        ws1.Range(Cells(pasterowindex, 1), (Cells(pasterowindex, 7))) = ws2.Range(Cells(fRow, 1), (Cells(fRow, 7)))
    End If
Next


But with better variables. I also dislike Do loops and stick to For Each loops, but that's my preference. It also can avoid the With block - you're nesting too many things, I think.

You also missed declaring these: dmr, pasteRowIndex

But I don't see you ever actually using dmr?

When you don't define your variable, VBA will declare it as a Variant, which are objects:


  Performance. A variable you declare with the Object type is flexible
  enough to contain a reference to any object. However, when you invoke
  a method or property on such a variable, you always incur late binding
  (at run time). To force early binding (at compile time) and better
  performance, declare the variable with a specific class name, or cast
  it to the specific data type.


By not declaring variables, you could possibly be paying a penalty.

Always turn on Option Explicit. You can have it automatically by going to Tools -> Options in the VBE and checking the Require Variable Declaration option. This way if you have any variables not defined, the compiler will let you know.

Additionally, I have no way of telling what dmr is. Plus first it's a workbook and then it's a range - the only reason it's working is because you didn't declare it and it's inherently a variant. Get two variables with good names in there!

Plus, what's this?

dim ws2 as worksheet
Set ws2 = wb1.Sheets("MAIN SCREEN").Range("A5:G2000")


Setting a worksheet variable to a range? Also why does ws2 reference wb1 and vice-versa? I got really confused on what the source and target actually are.



Praise


Great job using vbNullString instead of " ".
Good job using a Long instead of Integer. Integers - integers are obsolete. According to msdn VBA silently converts all integers to long.
Good job following Standard VBA naming conventions




Misc

When you do this

 ws1.Range("A5:G2000").ClearContents


You already gave the book and sheet a variable, just give this a range variable - it will make the code look cleaner. Additionally if you're using 2000 because you don't know how big the range is, 
there is a standard way to find lastRow and lastColumn. That post explains why.

What is this spacing about - 

 Set Wb2 = Workbooks.Open("C:\Users\elhayani\Desktop\development\AML db\OfficerA.xlsx")
  Set wb1 = Workbooks.Open("C:\Users\elhayani\Desktop\development\AML db\Mainwb.xlsm")
   Set ws2 = wb1.Sheets("MAIN SCREEN").Range("A5:G2000")
   Set ws1 = Wb2.Worksheets("DATA")


Be sure you align your levels:

Set Wb2 = Workbooks.Open("C:\Users\elhayani\Desktop\development\AML db\OfficerA.xlsx")
Set wb1 = Workbooks.Open("C:\Users\elhayani\Desktop\development\AML db\Mainwb.xlsm")
Set ws2 = wb1.Sheets("MAIN SCREEN").Range("A5:G2000")
Set ws1 = Wb2.Worksheets("DATA")


Also, what happens if the book isn't found or the sheet doesn't exist? Handle those errors!

Same goes for a space in the inputbox - " " &lt;&gt; vbNullString

Also why are you doing this -

strSearch = TextBox1.Value
...
strSearch = InputBox("Please enter T24 ID:", "Search Value")


I don't see that TextBox1 ever being used.



My version

Would look something like this -

Option Explicit

Sub CommandButton1_Click()

Dim sourceWorkbook As Workbook
Dim targetWorkbook As Workbook
Dim sourceSheet As Worksheet
Dim targetSheet As Worksheet
Dim sourceRange As Range
Dim targetRange As Range
Dim searchCell As Range
Dim searchValue As String
Dim targetRow As Long
targetRow = 5
Dim lastRow As Long

On Error GoTo ErrHandler

Set sourceWorkbook = Workbooks.Open("C:\Users\elhayani\Desktop\development\AML db\OfficerA.xlsx")
Set sourceSheet = sourceWorkbook.Worksheets("DATA")
lastRow = sourceSheet.Cells(Rows.Count, 1).End(xlUp).Row
Set sourceRange = sourceSheet.Range("A5:G" &amp; lastRow)

Set targetWorkbook = Workbooks.Open("C:\Users\elhayani\Desktop\development\AML db\Mainwb.xlsm")
Set targetSheet = targetWorkbook.Sheets("MAIN SCREEN")
Set targetRange = targetSheet.Range("A5:G" &amp; lastRow)
targetRange.ClearContents

searchValue = InputBox("Please enter T24 ID:", "Search Value")
If searchValue = vbNullString Or searchValue = " " Then Exit Sub

For Each searchCell In sourceRange 'or your specific range
    Set searchCell = sourceRange.Find(searchValue, LookIn:=xlValues)
    If Not searchCell Is Nothing Then
        targetSheet.Range(Cells(targetRow, 1), (Cells(targetRow, 7))) = sourceSheet.Range(Cells(searchCell.Row, 1), (Cells(searchCell.Row, 7)))
        targetRow = targetRow + 1
    End If
Next
Exit Sub

ErrHandler:
    MsgBox "WHOOPS"

End Sub




Advanced

Your way of doing the loop to .Find the search value is pretty complicated considering you can just .FindNext and avoid the loop altogether, but I know that's a lot to take in right now.