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

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 <> 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 - " " <> 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(xl

StackExchange Code Review Q#138485, answer score: 3