VBA Code

[Damo10]

Hi,

I have written a macro and would like to know if there is a better way of doing it or just tidy it up a bit. I am still learning and any help or guidance would be much apreciated.

Sub UpdateTotals()
Dim WS As Worksheet
Dim Full As String
Dim Half As String
Dim SH As String
Application.ScreenUpdating = False
Sheets("Report").Activate
Range("Totals").ClearContents
For Each WS In ActiveWorkbook.Worksheets
If WS.Name = "Report" Then GoTo Nextws
If WS.Name = "Main" Then GoTo Nextws
If WS.Name = "Summary" Then GoTo Nextws
WS.Activate
SH = ActiveSheet.Name
Full = Range("B5").Value
Half = Range("C5").Value
    With Sheets("Report")
        lr = .Range("D" & Rows.Count).End(xlUp).Row
        .Range("D" & lr + 1).Value = SH
        .Range("E" & lr + 1).Value = Full
        .Range("F" & lr + 1).Value = Half
        End With
Nextws:
Next WS
Sheets("Report").Activate
    Columns("D:D").Select
    With Selection
        .HorizontalAlignment = xlLeft
        .VerticalAlignment = xlBottom
    End With
        Columns("E:F").Select
    With Selection
        .HorizontalAlignment = xlCenter
        .VerticalAlignment = xlBottom
    End With
        Range("C2:J4").Select
    With Selection
        .HorizontalAlignment = xlCenter
        .VerticalAlignment = xlCenter
        .WrapText = False
        .Orientation = 0
        .AddIndent = False
        .IndentLevel = 0
        .ShrinkToFit = False
        .ReadingOrder = xlContext
        .MergeCells = True
    End With
    Range("A1").Select
Application.ScreenUpdating = True
End Sub

Regards Damian

 

[MrKowz]

A few things I noticed:

• Don't use Labels. They are generally not needed and can create spaghetti code, leading to confusion. Take care of these situations with either a If...Then...Else statement; or, in this case, a Select Case statement.
• Avoid "selecting" cells and "activating" worksheets. There is rarely ever a case when selecting/activating is needed.
• There was no need to create a variable for SH, Full, and Half since the code was only using those values once. Instead, directly store the values in the cell.
• LR was not explicitly declared. I recommend always declaring every variable to prevent possible errors.

Sub UpdateTotals()
Dim WS As Worksheet
Dim LR As Long
With Application
    .ScreenUpdating = False
    .Calculation = xlCalculationManual
    .EnableEvents = False
End With
Sheets("Report").Activate
Range("Totals").ClearContents
For Each WS In ActiveWorkbook.Worksheets
    Select Case WS.Name
        Case "Report", "Main", "Summary"
            'Do Nothing
        Case Else
            With Sheets("Report")
                LR = .Range("D" & Rows.Count).End(xlUp).Row
                .Range("D" & LR + 1).Value = WS.Name                'Sheet Name
                .Range("E" & LR + 1).Value = WS.Range("B5").Value   'Full
                .Range("F" & LR + 1).Value = WS.Range("C5").Value   'Half
            End With
    End Select
Next WS
With Sheets("Report")
    With .Columns("D:D")
        .HorizontalAlignment = xlLeft
        .VerticalAlignment = xlBottom
    End With
    With .Columns("E:F")
        .HorizontalAlignment = xlCenter
        .VerticalAlignment = xlBottom
    End With
    With .Range("C2:J4")
        .HorizontalAlignment = xlCenter
        .VerticalAlignment = xlCenter
        .WrapText = False
        .Orientation = 0
        .AddIndent = False
        .IndentLevel = 0
        .ShrinkToFit = False
        .ReadingOrder = xlContext
        .MergeCells = True
    End With
    .Range("A1").Select
End With
With Application
    .ScreenUpdating = True
    .Calculation = xlCalculationAutomatic
    .EnableEvents = True
End With
End Sub

 

[Damo10]

Hi MrKowz,

Thanks for your reply and your advice, what you have said makes sense and has helped me understand a better way to write VBA

Regards Damian

 

[MrKowz]

Hi MrKowz,

Thanks for your reply and your advice, what you have said makes sense and has helped me understand a better way to write VBA

Regards Damian

No problem, thanks for the feedback.