Macro / VBA: Run macro on sheets defined in a list

[tommychowdah]

Hello everyone,

Question regarding a macro I have, see pasted below.

I have an excel file with approximately 100 sheets, and would like to automatically format rows and columns for specific sheets based on cell values in row 1 and column a. I have a tab named "List" which is named as "Tab_NAMES" and list all the tabs I wish to run this macro on. The macro takes forever and I am not even sure if I have it written correctly. Any suggestions?

-----

Sub TEST_Hide_All()

Dim StartTime As Double
Dim MinutesElapsed As String
StartTime = Timer

Dim rngCell As Range
Dim strSheetActive As String: strSheetActive = ActiveSheet.Name
Application.ScreenUpdating = False

With ThisWorkbook.Worksheets("List")
For Each rngCell In .Range("B2:B100" & .Range("B" & .Rows.Count).End(xlUp).Row)
If Trim(rngCell.Value) <> vbNullString Then Call Hide(rngCell.Value)
Next rngCell
End With

Application.Goto ThisWorkbook.Worksheets(strSheetActive).Cells(1)
Application.ScreenUpdating = True
Set rngCell = Nothing

MinutesElapsed = Format((Timer - StartTime) / 86400, "m:ss")

MsgBox "The File is Beautiful! (" & MinutesElapsed & " minutes)", vbInformation

End Sub

-----

 

[Joe4]

This line is not doing what you think and hitting too many rows:

[COLOR=#333333]For Each rngCell In .Range("[/COLOR][COLOR=#ff0000][B]B2:B100[/B][/COLOR][COLOR=#333333]" & .Range("B" & .Rows.Count).End(xlUp).Row)[/COLOR]

Change it to this:

[COLOR=#333333]For Each rngCell In .Range("[/COLOR][COLOR=#ff0000][B]B2:B[/B][/COLOR][COLOR=#333333]" & .Range("B" & .Rows.Count).End(xlUp).Row)[/COLOR]

The way you had it written, if the last row was 222, you were telling it to loop through B2:B100222

 

[JackDanIce]

Is the range on List B2:Bx where x is the last used row on column B?

This isn't clear:

.Range("B2:B100" & .Range("B" & .Rows.Count).End(xlUp).Row)

It suggests Range("B2:B100" & 5000) if 5000 is your last used row, so you're actually looping through B2:B1005000, i.e. more cells than necessary, is this correct?

Looping with an Each object is slower than through the index of objects.
You can make more efficient by using For x = 2 to LR (where LR is the last row number) i.e.

For x = 2 to LR
    If Timr$(cells(x, 2).Value <> vbNullString Then Call Hide(Cells(x, 2).Value)
Next x

What does Hide(rngCell.Value) do? If you're not showing additional code, how should a reader suggest improvements (assuming rest of code is just looping)?

 

[tommychowdah]

WOW! You guys are awesome. Let me try this out and get back to you!

 

[tommychowdah]

Here is the additional Code.

The macro above calls a private sub

-----

Private Sub Hide(List As String)

Call Hide_All_Columns
Call Hide_All_Rows

End Sub

-----

Both of the additional macros called are the following.

-----

Sub Hide_All_Columns()

Dim c As Range
Dim ws As Worksheet


On Error GoTo err_handler
Application.ScreenUpdating = False


For Each ws In ActiveWorkbook.Worksheets
For Each c In ws.Rows("1:1").Cells
If c.Value = "1" Then
c.EntireColumn.Hidden = True
Else
c.EntireColumn.Hidden = False
End If
Next c
Next ws

Application.ScreenUpdating = True
Exit Sub


err_handler:
MsgBox "Sheet: " & ws.Name & " caused an error"


End Sub

-----

Sub Hide_All_Rows()


Dim r As Range
Dim ws As Worksheet


On Error GoTo err_handler
Application.ScreenUpdating = False


For Each ws In Worksheets
For Each r In ws.Range("A1:A1000")
If r.Value = 1 Then
r.EntireRow.Hidden = True
Else
r.EntireRow.Hidden = False
End If
Next r
Next ws

Application.ScreenUpdating = True
Exit Sub


err_handler:
MsgBox "Sheet: " & ws.Name & " caused an error"


End Sub

-----

Any thoughts?

 

[tommychowdah]

JackDanIce,

How would I incorporate your index code into mine?

Best,
Chowdah

 

[JackDanIce]

This code makes little sense.

You're passing a string variable into Hide but doing nothing with that variable, so what is the point of looping over the values in column B in List?

Hide_All_Columns then, with nothing to do with the string variable passed to Hide, is called and loops over every sheet in the activeworkbook (including the activesheet), tests each non-blank cell in row 1 for a string value of "1" (this is different to the number value 1) and if exists, hides the column), otherwise unhides it. If an error is encountered, a msgbox is displayed but no other action is taken.

Hide_All_Rows is like above, except looping through a fixed range A1:A1000 of every sheet in the same workbook, hiding rows if the number value (why change from string "1"?) 1 exists, if so, the row is hidden, otherwise make it visible, again with the same error message returned but no other action.

What is the purpose of this macro, you have unecessary looping and testing with no clear logic or connections as far as I can tell.

No detail of contents of column B in list (your screen is not visible)
No explanation of what the sheet name is or what the other sheets in the workbook are
No explanation of why the columns/rows are hidding/unhidding and a difference of testing for "1" vs 1
No explanation for why an unspecified number of columns are tested, but a fixed count of 1000 rows between the two private sub-procedures

It's slow because it hasn't been designed well from what it appears to be doing.

To index column B of sheet List, try:

Sub THA()

    Dim x   As Long
    Dim LR  As Long
    Dim w   As Worksheet: Set w = ActiveSheet
    
    Application.ScreenUpdating = False
    
    With Sheets("List")
        LR = .Cells(.Rows.Count, 2).End(xlUp).Row
        For x = 2 To LR
            With .Cells(x, 2)
                If Len(Trim$(.Value)) Then Hide (.Value)
            End With
        Next x
    End With
    
    With Application
        .Goto w.Cells(1, 1), True
        .ScreenUpdating = True
    End With
    Set w = Nothing
    
End Sub