3
\$\begingroup\$

Here is some code which loops through an area in a spreadsheet and executes the code based on the condition that the source cells do not contain the value "(blank)".

The code works, but it's very inefficient to run nested if statements in this manner. I've tried to have a go at making it more efficient in the long run but I'm out of ideas.

Any suggestions?

Sub NestedIfStatement()
Dim lastrow1 As Long
Dim I As Integer, J As Integer, N As Integer, MaxPriority as Integer

N = 3
Set Maxnumber = WS1.Range("A" & I & ":A" & lastrow1)
    MaxPriority = Application.Max(Maxnumber)

For J = 1 To lastrow1
    If WS1.Cells(J, 1) <= MaxPriority Then
       If WS1.Cells(J, 6).Value <> "(blank)" Then
          WS3.Cells(N, 7).Value = WS1.Cells(J, 6).Value
       End If
       If WS1.Cells(J, 5).Value <> "(blank)" Then
          WS3.Cells(N, 6).Value = WS1.Cells(J, 5).Value
       End If
       If WS1.Cells(J, 4).Value <> "(blank)" Then
          WS3.Cells(N, 4).Value = WS1.Cells(J, 4).Value
       End If
       If WS1.Cells(J, 3).Value <> "(blank)" Then
          WS3.Cells(N, 3).Value = WS1.Cells(J, 3).Value
       End If
       If WS1.Cells(J, 2).Value <> "(blank)" Then
          WS3.Cells(N, 2).Value = WS1.Cells(J, 2).Value
       End If
       N = N + 1
    End If
Next J

End Sub
\$\endgroup\$
0

1 Answer 1

3
\$\begingroup\$

Suggestion #1: Use Option Explicit and/or use fewer global variables

Either MaxNumber, MaxPriority are never explicitly defined, which is a very bad idea and is why you should use Option Explicit, or they're Global Variables, which is redundant, because they're self-contained within this subroutine.

Suggestion #2: Don't work directly with worksheet ranges

Accessing a worksheet is slow. You don't notice because doing it once is almost-instant, but do it a thousand times and it starts adding up really fast.

Whenever I'm working with worksheet data, the first thing I do is read all of it into an Array:

Dim wsData As Variant
    wsData = Array()

Dim dataTable As Range
Set dataTable = "" '/ Define your worksheet range

    ReDim arrWsData(1 To Rows, 1 To columns)  '/ Same size as the range

    wsData = dataTable

Now you have a 2-dimensional array, with all your worksheet data laid out in exactly the same way as it was on the sheet. Excel can iterate over an Array orders of magnitude faster than over a range because it doesn't have to access the worksheet object every time.

You can print an array to a sheet by doing the same in reverse: Define your range to be the same size as your array. Range = Array.

Suggestion #3: Recursion

Assuming you put the range A1:F50 from WS1 in an array, you can now iterate over it like so:

Dim i As Long, j As Long, n As Long
Dim testValue As Variant

Dim printData As Variant
    printData = Array()

ReDim printData(1 To lastRow, 1 To 5)

For j = 1 To lastRow
    If wsData(j, 1) <= MaxPriority Then
        n = n + 1
        For i = 1 To 5
            testValue = wsData(j, i + 1)
            If testValue <> "(blank)" Then printData(j, i) = testValue
        Next i
    End If
Next j

you can now change the size of your data, or the progression of the sequence, or a whole host of other things by changing 1 or 2 numbers. And 20 lines of code just became 4.

You can now pick any range on WS3 and print your output data there.

I don't know why your output columns on WS3 Jump a column from 6 to 4, but you can always write specific parts of your array to specific parts of the output sheet if needed.

Suggestion #4: Meaningful Variable Names

What is WS1? What is WS3? I know they're worksheets, but which worksheets? They appear next to each other in multiple places in this subroutine. If you accidentally put a 1 instead of a 3 somewhere, you can't tell at a glance where you've gone wrong. Give your variables names

Even a simple wsInput, wsOutput would be so much better.

MaxNumber Sounds like it should be, well, a number. Except it's a range. variables should say what they are. Even if that means writing VariableThatHoldsThisThing, it'll save you so much time down the road in debugging and maintaining and less technical debt.

Here, I recommend something like dataTable<description> (I don't know the context of your data, so you can think of a much more succinct name yourself).

On the other side, MaxPriority is a good variable name. It holds a number which represents the maximum priority from some table of priority numbers.

Also NestedIfStatement is a terrible procedure name. It tells me nothing about what the procedure does. Procedure names should be of the form <DoThing> or Perfom<Action>On<Thing> or any larger combination thereof. If you can't describe your procedure in a (reasonably) short name, then it's a good sign that it's trying to do too much, and should be split into several smaller procedures.

\$\endgroup\$
0

Your Answer

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.