0
\$\begingroup\$

I have code that is identical minus two changes that would happen each time, which I can use variable for, but I don't want to use a bunch of if statements.

C = 1
fnd = Sheet1.Cells(C, 8)
While InStr(fnd, "Customer Name:") <= 0 And C <= 300
C = C + 1
fnd = Sheet1.Cells(C, 8)
Wend
If C >= 300 Then
Sheet2.Cells(r, 2) = "Not found"
Else
cleanup = InStr(1, fnd, ": ")
finalString = Right(fnd, Len(fnd) - cleanup - 1)
Sheet2.Cells(r, 2) = finalString
End If

What would be changing is where it says "Customer Name" would have to go through 8 other variables, at this point, and the constant number that's in the sheet.cells line would need to increase by one each time.

I know I can just use a counter and increase that counter for the column number in sheet 2 each time the block of code runs, but as far as making Customer Name change to customer email, customer points, etc, I'm not sure how to use a variable for that without doing it the long (and probably wrong) way of

If i=1 then
sch="Customer Name"
Elseif i=2 then
sch="Customer Email"

and on and on down the list.

Right now I have that original block of code showing up 8 times, and only those two things are changing, so it's really a waste of storage space (not that it's really anything noticeable), so I'm thinking there has to be a better way than how I have it or setting it up with a bunch of If/Then statements.

Jamal
35.2k13 gold badges134 silver badges238 bronze badges
asked Oct 20, 2015 at 17:52
\$\endgroup\$
4
  • 2
    \$\begingroup\$ We all want to clean up our code and make it more readable. To make your title more unique, try to let it say what your code is about, not what kind of answers you are looking for. \$\endgroup\$ Commented Oct 20, 2015 at 18:00
  • 4
    \$\begingroup\$ It will be better to include a bigger piece of your program, so that we can better see its overall structure and methods. Good answers will likely involve some restructuring and alternative/better method decomposition in order to reduce the duplicated logic. \$\endgroup\$ Commented Oct 20, 2015 at 18:05
  • 1
    \$\begingroup\$ Are these variables declared anywhere? Are you using Option Explicit? Is this code in a Sub or in a Function? Is that procedure doing more than just that? Please include more of the context around the specific code you're interested in improving - don't worry about making a long post, this site is quite different from Stack Overflow where they like everything boiled-down and examplified. \$\endgroup\$ Commented Oct 20, 2015 at 18:23
  • 3
    \$\begingroup\$ The desire to improve code is implied for all questions on this site. Question titles should reflect the purpose of the code, not how you wish to have it reworked. See How to Ask. \$\endgroup\$ Commented Oct 20, 2015 at 19:15

2 Answers 2

5
\$\begingroup\$

Your looping code can be made smoother and more efficient. Try this instead. It sets the range to check from the beginning, and lets you use a string array named CustomerFields for your Customer Name, Customer E-mail, etc.:

For Loop1 = LBound(CustomerFields) to UBound(CustomerFields)
 With Sheet1
 Set Range1 = .Range(.Cells(1, 8), .Cells(300, 8)
 For Each Cell1 in Range1
 If InStr(1, Cell1.Value, CustomerFields(Loop1), vbtextcompare) > 0 Then
 cleanup = InStr(1, Cell1.Value, ":")
 finalString = Right(Cell1.Value, Len(Cell1.Value) - cleanup - 1)
 Sheet2.Cells(r, 2) = finalString
 End If
 Next Cell1
 End With
Next Loop1
answered Oct 20, 2015 at 18:52
\$\endgroup\$
5
\$\begingroup\$

You'll have to either manually type a list of terms into VBA, or have them written in a sheet and pull them into an array/collection. Assuming the former: I'd have a separate procedure like so:

Public Sub FillSearchStrings(ByRef arrayList As Variant)
 Dim ix As Long
 ix = 0
 arrayList = Array()
 ix = ix + 1
 ReDim Preserve arrayList(1 to ix)
 arrayList(ix) = "Customer Name"
 ix = ix + 1
 ReDim Preserve arrayList(1 to ix)
 arrayList(ix) = "Customer Email"
 ...
End Sub

So you'll always know where to go if you need to change it.

Then you call it from your main procedure like so:

Dim searchStrings As Variant
 FillSearchStrings searchStrings

And then we determine the size of our list, and iterate over it like so:

Dim LB1 As Long, UB1 As Long
 LB1 = LBound(searchStrings)
 UB1 = UBound(searchStrings)
Dim ix As Long, searchTerm As String
 For ix = LB1 To UB1
 searchTerm = searchStrings(ix)
 ...

Now, for your looping, I'd recommend putting your column in an array first (best practice for all static spreadsheet data) and then looping through your array:

Dim sheetData As Variant
 sheetData = Array()
Dim dataCol As Long
 dataCol = ix + 7
Dim dataRange As Range
 Set dataRange = Range(Cells(1, dataCol), Cells(300, dataCol))
 sheetData = dataRange ' now sheetdata(1) = row 1 etc.
 Dim iy As Long, stringToSearch As String
 Dim matchIsFound As Boolean, matchPosition As Long
 matchIsFound = False
 iy = LBound(sheetData) + 1
 Do While iy <= UBound(sheetData) And matchIsFound = False
 stringToSearch = CStr(sheetData(iy)) ' converts our data to string form
 matchPosition = InStr(1, stringToSearch, searchTerm, vbTextCompare) 'vbBinaryCompare for case-Sensitive comparison
 If matchPosition > 1 Then matchIsFound = True
 iy = iy + 1
 Loop

Now, we can easily add terms, modify our targeted ranges, add new behaviour etc. and the code will automatically adjust. Plus, it's much clearer what everything is doing.

answered Nov 30, 2015 at 13:40
\$\endgroup\$

Your Answer

Draft saved
Draft discarded

Sign up or log in

Sign up using Google
Sign up using Email and Password

Post as a guest

Required, but never shown

Post as a guest

Required, but never shown

By clicking "Post Your Answer", you agree to our terms of service and acknowledge you have read our privacy policy.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.