0

Below is the code that I'm running, I'm not too sure if I can get what I want.
But strangely, the code is taking too long to run.
I was waiting for it for an hour.
So I figured there must be some errors in the code.
Can anybody shed some light on this?

Set x = Workbooks.Open("C:\Users\Desktop\testing.xlsx")

For Each ws In x.Worksheets
If ws.Name <> "Master" Then
    lrow = ws.Cells(Rows.Count, "A").End(xlUp).Row
    Set rng = ws.Range(Cells(1, 1), Cells(lrow, 1))
    For Each Acell In rng
        If (Acell.Value = "Sum") Then
            Acell.Offset(-1, 0).EntireRow.Insert
        End If
    Next Acell
End If
Next ws

I'm looking for that "Sum" word in Col A, for every sheets in workbook except "master" sheet.

Santa
  • 47
  • 14
  • Will the "sum" word appear more than once in a column? – Hank Jun 20 '17 at 06:54
  • 1
    When deleting or inserting rows, (especially before the current row), you should always go from the last line to the first one, in reverse order. – Vincent G Jun 20 '17 at 06:57
  • 1
    And you should specify **always EVERY** `Cells`, `Rows`, `Range` etc with your worksheet (and you didn't) like `ws.Cells`, `ws.Rows`, `ws.Range`. **Never use them without qualifying the worksheet.** If you don't qualify the worksheet, then Excel always assumes you mean the `ActiveSheet` instead (which is mostly wrong). Read [VBA Best Practices: Never Assume the Worksheet](https://stackoverflow.com/documentation/excel-vba/1107/vba-best-practices/9218/never-assume-the-worksheet) for better understanding. – Pᴇʜ Jun 20 '17 at 07:06
  • @Hank, nope. There is only 1 "sum" word in the column. – Santa Jun 20 '17 at 07:15
  • If there is only one `sum` in each sheet just insert a `Exit For` after `Acell.Offset(-1, 0).EntireRow.Insert` – Pᴇʜ Jun 20 '17 at 07:15
  • @VincentG, sorry i'm kinda new to vba, doesn't (xlUp) going from last to the first ? – Santa Jun 20 '17 at 07:18
  • @Peh, thanks for the link and pointing out mistake in the code. I'm going through the link and see how can i fix them – Santa Jun 20 '17 at 07:20
  • Kinda yes but what @VincentG meant was looping from bottom to top. Instead of the `For Each` loop using a `For i = lrow to 1 Step -1` and then using `i` to access each row: `If (ws.Cells(i, 1) = "Sum") Then ws.Rows(i).Insert`. When looping from the bottom, then inserting/deleting rows doesn't affect rows above (which are not processed yet) but just rows below (which are already processed). – Pᴇʜ Jun 20 '17 at 07:25
  • For speed your best bet is to use arrays. I havnt got time to write you some code but your pseudo code would read like this -open xls -loop sheets -read range into array -loop array -when sum found add blank element into array (google can help with this) -once loop done read array back out to the right column -next sheet – 99moorem Jun 20 '17 at 09:26

5 Answers5

0

When you insert a row and then continue with the loop you will end up on a "Sum"-row again (it has been pushed one row down due to the Insert). Therefore your code will insert row after row after row after row... forever

Try this code (inside the If)... not the best solution perhaps, but it works:

lastrow = ws.Cells(Rows.Count, "A").End(xlUp).Row
R = 1
Do Until R > lastrow
    If (ws.Cells(R, 1).Value = "Sum") Then
        ws.Cells(R, 1).EntireRow.Insert
        R = R + 1             ' Jump one extra step (to skip the inserted row)
        lastrow = lastrow+ 1  ' The last row is increased due to the inserted row
    End If
    R = R + 1
Loop
Gowire
  • 1,046
  • 6
  • 27
  • 1
    You can avoid this by looping from bottom to top (going upwards). Especially this might be faster if the "sum" is bottom located and only occurring once so we can exit the loop after the first match. – Pᴇʜ Jun 20 '17 at 07:45
0

Since there is only 1 sum in a column

Set x = Workbooks.Open("C:\Users\Desktop\testing.xlsx")

For Each ws In x.Worksheets
If ws.Name <> "Master" Then
    lrow = ws.Cells(Rows.Count, "A").End(xlUp).Row
    Set rng = ws.Range(Cells(1, 1), Cells(lrow, 1))
    For Each Acell In rng
        If (Acell.Value = "Sum") Then
            Acell.Offset(-1, 0).EntireRow.Insert
        exit for
        End If
    Next Acell
End If
Next ws

try this code.

Hank
  • 187
  • 1
  • 1
  • 13
0

You are looping through the worksheets but not using each individual worksheet as the parent worksheet to the cells. Without a qualified parent worksheet, each Cells simply defaults to the ActiveSheet.

For Each ws In x.Worksheets
    If ws.Name <> "Master" Then
        with ws
            lrow = .Cells(.Rows.Count, "A").End(xlUp).Row
            Set rng = .Range(.Cells(1, 1), .Cells(lrow, 1))
            For Each Acell In rng
                If Acell.Value = "Sum" Then
                    Acell.Offset(-1, 0).EntireRow.Insert
                End If
            Next Acell
        end with
    End If
Next ws

Note that you are starting in row 1 and if A1 is Sum then you are trying to insert a row at row 0. There is no row zero.

  • related: [Is the . in .Range necessary when defined by .Cells?](https://stackoverflow.com/questions/36368220/is-the-in-range-necessary-when-defined-by-cells) –  Jun 20 '17 at 07:17
0

Instead of the For Each loop use a For i = lRow to 1 Step -1 and then use i to access each row: If ws.Cells(i, 1) = "Sum" Then ws.Rows(i).Insert.

When looping from the bottom, then inserting/deleting rows doesn't affect rows above (which are not processed yet) but just rows below (which are already processed).

Dim iRow As Long, lRow As Long
Dim ws As Worksheet
Dim x As Workbook

Set x = Workbooks.Open("C:\Users\Desktop\testing.xlsx")

For Each ws In x.Worksheets
    If ws.Name <> "Master" Then
        lRow = ws.Cells(ws.Rows.Count, "A").End(xlUp).Row
        For iRow = lRow To 1 Step -1 'walk through rows from last row (lRow) to first row going upwards (Step -1)
            If ws.Cells(iRow, 1) = "Sum" Then
                ws.Rows(iRow).Insert xlDown 'insert new row and move other rows down
                Exit For  'if there is only one "Sum" you can exit for here (to shorten the run time) 
                          'if there are more that one "Sum" in a sheet then remove that line.
            End If
        Next iRow
    End If
Next ws
Pᴇʜ
  • 56,719
  • 10
  • 49
  • 73
-2

Have you tried setting Workbook Calculation to Manual, i found this helpful since I tend to work with workbooks with a load of sheets an every sheets has a load of formulas.

File>Options>Formulas

then set Workbook Calculations to manual, but if you want to automate this.

Sub Macro1()
'
' Macro1 Macro
'
' Keyboard Shortcut: Ctrl+e
'
    calcu
    'your code here

    ActiveWorkbook.Application.Calculation  = xlCalculationAutomatic

End Sub

'=

Function calcu()

    Dim xlCalc As XlCalculation

    xlCalc = Application.Calculation
    ActiveWorkbook.Application.Calculation = xlCalculationManual
    On Error GoTo CalcBack
    Exit Function

CalcBack:
    ActiveWorkbook.Application.Calculation = xlCalc

End Function

I got this from somewhere in stack but I forgot which post, so if you're the one who did this, thanks. :)

xtoybox
  • 57
  • 7
  • Calculation has nothing to do with the actual issue here I think. – Pᴇʜ Jun 20 '17 at 07:46
  • Seem so, looking at it again, it seems user is looping through all the cells on every sheet. Do you think using Range.Find would be a lot better? – xtoybox Jun 20 '17 at 08:03