3
\$\begingroup\$

I am trying to find certain strings in MasterSheet and copy offset columns (3 columns) values in the specific columns in receiver sheet. So far i have this code doing exactly what i want to do

Sub FindString()

Dim MasterSheet As Workbook     'Master sheet where data is coming from
Dim WSLHD As Worksheet          'Slave sheet receiving the data from mastersheet
Dim Findstring1 As Range        'Transfer <= 30 minutes
Dim Findstring2 As Range        'End of Month Performance (EMP)



Set MasterSheet = Workbooks.Open("\\mypath\test.xlsx")
Set WSLHD = ThisWorkbook.Worksheets("WSLHD")

'list of strings which will be searched for (this are just examples, there are many more strings)

Set Findstring1 = MasterSheet.Sheets("sheet1").Cells.Find(what:="Transfer <= 30 minutes")
Set Findstring2 = MasterSheet.Sheets("sheet1").Cells.Find(what:="End of Month Performance (EMP)")

'copy values from offset columns
WSLHD.Range("B8").Value = Findstring1.Offset(0, 1).Value
WSLHD.Range("C8").Value = Findstring1.Offset(0, 2).Value
WSLHD.Range("F8").Value = Findstring1.Offset(0, 3).Value

WSLHD.Range("B9").Value = Findstring2.Offset(0, 1).Value
WSLHD.Range("C9").Value = Findstring2.Offset(0, 2).Value
WSLHD.Range("F9").Value = Findstring2.Offset(0, 3).Value

MasterSheet.Close

end Sub

.. but i want to do this in more efficient way..i did try to use filter but that will be again filtering and choosing texts one by one so wont be any difference in terms of efficiency. Any help will be appreciated.

\$\endgroup\$
6
\$\begingroup\$

Option Explicit is missing from your code. From the menu at the top under Tools>Options>Editor tab>Code Settings group>Require Variable Declaration <-- toggle that check box on. It will thereafter add the Option Explicit statement to all modules. You'll have to add this manually to any currently existing modules. This mandates you have Dim foo as Range to declare your variables before usage. Not having this leads to avoidable errors. Turn it on and leave it on. Future-you will thank you for doing so.

Your Sub statement is implicitly Public since it is missing the access modifier. Writing Public Sub FindString() makes in known that you intentionally made it publicly viewable.

Indentation. Indenting your code by a tab helps identify where a Sub begins and ends. Same goes for blocks of code such as If...End IF.

Variable names are typically Camel Case. This means the first word is lower cased and the first letter of subsequent words is upper cased. If you had a variable worksheetContainingWordsToFind that would be an example of came Camel Case. Your Sub name FindString is in Pascal Case where the first letter of every word is upper cased. Subs/Function are Pascal Case while variables are Camel Case.

Your variable MasterSheet is misleading. It's actually a Workbook. Have your variables be what they say they are. Dim MasterSheet As Worksheet is an improvement to match up with what it says it is. Even better for it to be sourceSheet. That way you can have

Dim sourceBook as Workbook
Set sourceBook as Workbooks.Open("\\mypath\text.xlsx")

Dim sourceWorksheet as Worksheet
set sourceWorksheet = sourceBook.Worksheets("sheet1")

Dim foundTransferString as Range
Set foundTransferString = sourceWorksheet.Cells.Find(What:="Transfer <= 30 minutes")

Dim foundEMPString as Range
Set foundEMPString = sourceWorksheet.Cells.Find(What:="End of Month Performance (EMP)"

which has your code self documenting.

The above code also eliminates a wall of declarations at the start. I'm of the opinion that declaring a variable just before you use it helps aid in refactoring and eliminating unused variables. Rather than needing to right click on the variable to display the context menu>Definition to find where it's declared you have it right above the first use.


The code ThisWorkbook.Worksheets("WSLHD") can use the object (Excel Worksheet) directly. In the Project Explorer, found in the menu at the top under View>Project Explorer (Hotkey: Ctrl+R). Double click the sheet under Microsoft Excel Object to display the code behind pane for that Worksheet object. Under View>Properties Window (Hotkey: F4) display the properties window. At the very top where it says (Name) you can rename the worksheet CodeName to a more descriptive name. I kept WSLHD as I don't know what that means, another reason to have self describing names. Thereafter you can use that object directly with WSLHD.Cells.Find instead of in the roundabout way of assigning a worksheet variable with ThisWorkbook.Worksheets("WSLHD"). The latter is prone to breaking if the tab is renamed.


Your comment "... there are many more strings" is a sign-post indicating you want a function. This function has 2 input variables that you provide as arguments. It searches the sheet you indicate looking for the string value. If it finds the result it sets the last variable outFoundCell which is passed ByRef so the assignment applies is reflected in the calling procedure. If the string was found the TryFind = Not outFoundCell Is Nothing assigns True to the function, otherwise it assigns False.

Private Function TryFind(ByVal searchForValue As String, ByVal sourceSheet As Worksheet, ByRef outFoundCell As Range) As Boolean
    Set outFoundCell = sourceSheet.Cells.Find(What:=searchForValue)
    TryFind = Not outFoundCell Is Nothing
End Function

You couple that with a method to populate the cells

Private Sub PopulateCellsWithOffsetResults(ByVal foundCell As Range, ByVal populationSheet As Worksheet, ByVal populationRow As Long)
    populationSheet.Cells(populationRow, "B").Value2 = foundCell.Offset(0, 1).Value2
    populationSheet.Cells(populationRow, "C").Value2 = foundCell.Offset(0, 2).Value2
    populationSheet.Cells(populationRow, "F").Value2 = foundCell.Offset(0, 3).Value2
End Sub

And you can then invoke the function in the following manner. You first search for the string. If it's found it then enters the true part of the If...Then...Else Statement and populates the information.

If TryFind("Transfer <= 30 minutes", sourceWorksheet, foundCell) Then
    PopulateCellsWithOffsetResults foundCell, WSLHD, 8
End If

Doing all that you end up with the code below. There is more that can be done because the population rows 8, and 9 are static numbers. You probably want a dynamic row that changes as things are populated.

Option Explicit

Public Sub SearchForStringsAndPopulatedOffsetValuesOnAnotherWorksheet()
    Dim sourceBook As Workbook
    Set sourceBook = Workbooks.Open("\\mypath\test.xlsx") 'Master sheet where data is coming from
    Dim sourceWorksheet As Worksheet
    Set sourceWorksheet = sourceBook.Sheets("sheet1")

    Dim foundTransferCell As Range
    If TryFind("Transfer <= 30 minutes", sourceWorksheet, foundTransferCell) Then
        PopulateCellsWithOffsetResults foundTransferCell, WSLHD, 8
    End If

    Dim foundEndOfMonthPerformanceCell As Range
    If TryFind("End of Month Performance (EMP)", sourceWorksheet, foundEndOfMonthPerformanceCell) Then
        PopulateCellsWithOffsetResults foundEndOfMonthPerformanceCell, WSLHD, 9
    End If

    sourceBook.Close
End Sub

Private Sub PopulateCellsWithOffsetResults(ByVal foundCell As Range, ByVal populationSheet As Worksheet, ByVal populationRow As Long)
    populationSheet.Cells(populationRow, "B").Value2 = foundCell.Offset(0, 1).Value2
    populationSheet.Cells(populationRow, "C").Value2 = foundCell.Offset(0, 2).Value2
    populationSheet.Cells(populationRow, "F").Value2 = foundCell.Offset(0, 3).Value2
End Sub

Private Function TryFind(ByVal searchForValue As String, ByVal sourceSheet As Worksheet, ByRef outFoundCell As Range) As Boolean
    Set outFoundCell = sourceSheet.Cells.Find(What:=searchForValue)
    TryFind = Not outFoundCell Is Nothing
End Function

The Sub PopulateCellsWithOffsetResults and Function TryFind are Private and thereby not visible outside of the module because they are an implementation detail. You aren't concerned with how they work, rather that they do work as evidenced when you run SearchForStringsAndPopulatedOffsetValuesOnAnotherWorksheet which should have a better more descriptive name. That's just what I came up with since I really don't know everything that you're populating.

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ Thank you so much for not only review and suggestions but also for the detailed explanation, this is the most detailed/well explained answer i have ever came across on stack exchange community. I will try out all the suggestion you have mentioned and certainly let you know how I go with the new script. Thanks a ton. \$\endgroup\$ – victor Jan 2 at 3:46
  • 2
    \$\begingroup\$ Be aware that in the Range.Find remarks section it states "The settings for LookIn, LookAt, SearchOrder, and MatchByte are saved each time you use this method. If you do not specify values for these arguments the next time you call the method, the saved values are used." so you may want to explicitly set them in the TryFind function. TryFound could be renamed TryToFind but I'll leave that for you. \$\endgroup\$ – IvenBach Jan 2 at 7:13
  • \$\begingroup\$ i am getting "Compile error: Argument not optional" on Dim foundTransferCell As Range If TryFind("Transfer <= 30 minutes", sourceWorksheet, foundTransferCell) Then PopulateCellsWithOffsetResults foundTransferCell, WSLHD, 8 End If just before the End if line \$\endgroup\$ – victor Jan 8 at 0:59
  • \$\begingroup\$ I tried changing WSLHD sheet name to populationSheet but it didnt clear the error \$\endgroup\$ – victor Jan 8 at 2:13
  • 1
    \$\begingroup\$ WSLHD needs to be the CodeName of a worksheet object. Alternatively declare it as a worksheet with Dim WSLHD As Worksheet and then assign a reference to that variable with Set WSLHD = ThisWorkbook.Worksheets("WSLHD") as in your original post. Having the CodeName property of a worksheet object already set to WSLHD avoids this declaration and assignment, which is what tried to explain after the first horizontal line in my answer. My apologies if I wasn't clear enough. \$\endgroup\$ – IvenBach Jan 8 at 3:42

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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