0
\$\begingroup\$

I write decently sloppy code and piecemeal together various things I find online to help me accomplish what needs to be done. This loop goes through 4 different worksheets to paste a formula in 3 columns. Based on some previous feedback from the site, I removed all of my .Select references and hopefully did things more cleanly. 2 questions that I have:

1) Can anyone think of a way I can further optimize this to have it run more quickly?

2) Is there a better way to define my RRSData!A2:O10000 formula. I just chose 10,000 because there shouldn't be that many in the future, but its obviously working harder than it needs to now since there are only ~1000 rows currently.

Dim LastRow1 As Long, i1 As Long, InxW As Long
Dim wsNames As Variant

wsNames = Array("Sheet1", "Sheet2", "Sheet3", "Sheet4")

For InxW = LBound(wsNames) To UBound(wsNames)
   With Worksheets(wsNames(InxW))
        LastRow = Cells(Rows.Count, "B").End(xlUp).Row
        For i1 = 25 To LastRow
            With Range("KA1").Offset(i1 - 1, 0)
                .Formula = "=IF(SUMIF(RRSData!$A$2:$O$10000,OFFSET($KA$1,CELL(""row"",THIS),-285),RRSData!$M$2:$M$10000)>0,SUMIF(RRSData!$A$2:$O$10000,OFFSET($KA$1,CELL(""row"",THIS),-285),RRSData!$M$2:$M$10000),"""")"
                .NumberFormat = "0.00"
                .Value = .Value
            End With
            With Range("KB1").Offset(i1 - 1, 0)
                .FormulaR1C1 = "=IF(RC[-1]="""","""",If(RC[-1]>1.1,""High"",If(RC[-1]<0.8,""Low"",""Neutral"")))"
                .Value = .Value
            End With
            With Range("KC1").Offset(i1 - 1, 0)
                .FormulaR1C1 = "=IF(RC[-2]="""","""",IF(RC[-2]>1.1,""+5%"",IF(RC[-2]<0.8,""-5%"",""0%"")))"
                .Value = .Value
            End With
        Next i1
    End With
Next InxW

The code works, but it takes probably 2 minutes to loop through the four sheets, and the total number of rows across the four sheets is <500

\$\endgroup\$
  • 1
    \$\begingroup\$ What is the aim of your code? Your title doesn't state that. In the body of your text you can state that looping is slow and with a descriptive title we'd have a better idea of what you're doing and how to best help. codereview.meta.stackexchange.com/questions/2436/… is one of the links available in the top right when you click the Ask Question button. With more information we can give you a better and more thorough review. \$\endgroup\$ – IvenBach May 6 '19 at 20:08
  • \$\begingroup\$ @IvenBach Thanks for the tip, I have updated the title to be more descriptive. \$\endgroup\$ – nikedude May 6 '19 at 21:15
2
\$\begingroup\$

A couple of tips.

When using With, make sure that you correctly reference the subordinate lines. Check the following example.

With Range("KA1").Offset(i1 - 1, 0) '<--- You have ...
With .Range("KA1").Offset(i1 - 1, 0) ' ... should be

Consider using FillDown or some similar command to do bulk edits. This will save looping, especially the loop that flips between the VBA engine and the Excel engine (which is expensive time-wise). Normally I suggest using arrays, but your use of formula suggests this other way.

The following example has not been tested.

Dim formulaOneRange as Range
    Set formulaOneRange = .Range("KA25:KA" & lastRow)
    With formulaOneRange
        .Formula = "=IF(SUMIF(RRSData!$A$2:$O$10000,OFFSET($KA$1,CELL(""row"",THIS),-285),RRSData!$M$2:$M$10000)>0,SUMIF(RRSData!$A$2:$O$10000,OFFSET($KA$1,CELL(""row"",THIS),-285),RRSData!$M$2:$M$10000),"""")"
        .NumberFormat = "0.00"
        .Value = .Value
    End With
    Set formulaOneRange = .Range("KB25:KB" & lastRow)
    With formulaOneRange
        .FormulaR1C1 = "=IF(RC[-1]="""","""",If(RC[-1]>1.1,""High"",If(RC[-1]<0.8,""Low"",""Neutral"")))"
        .Value = .Value
    End With
    Set formulaOneRange = .Range("KC25:KC" & lastRow)
    With formulaOneRange
        .FormulaR1C1 = "=IF(RC[-1]="""","""",If(RC[-1]>1.1,""High"",If(RC[-1]<0.8,""Low"",""Neutral"")))"
        .Value = .Value
    End With

Of course, the code could be tidies up a bit as per below, but I thought the extra lines will show my thinking a bit more clearly.

' My example code above ... 
Set formulaOneRange = .Range("KC25:KC" & lastRow)
With formulaOneRange

' Could be ....
With .Range("KC25:KC" & lastRow)

Also, defining the range in an easy to manage variable would assist if you wanted to use FillDown or do some other manipulation within each section.

\$\endgroup\$

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.