0

I have this code that obviously use Select, .Activate,...and I understand it's not a good practice in addition the application is craching now and then so thats probably because of using Select...

I'm pretty new to VBA and would appreciate help on how to change this code to NOT use Select.Activate, ActiveSheet,ActiveCell and maybe other consideration in order to get it more efficient.

  Sub FormatText()

    Sheets("A4").Cells(1 + PageRowOffset(Page) + BoxRowOffset(Box) - 2, BoxColOffset(Box)).Activate

    With ActiveCell.Font
        .Name = "Calibri"
        .Size = 11
        .Underline = xlUnderlineStyleNone
        .ThemeColor = xlThemeColorLight1
        .TintAndShade = 0
        .ThemeFont = xlThemeFontMinor
        .Bold = False
    End With



    With Range(Cells(PageRowOffset(Page) + BoxRowOffset(Box), 1 + BoxColOffset(Box)), Cells(PageRowOffset(Page) + BoxRowOffset(Box) + 3, 1 + BoxColOffset(Box) + 1)).Font
        .Name = "Calibri"
        .Size = 8
        .Strikethrough = False
        .Superscript = False
        .Subscript = False
        .OutlineFont = False
        .Shadow = False
        .Underline = xlUnderlineStyleNone
        .TintAndShade = 0
        .ThemeFont = xlThemeFontMinor
        .Bold = False
    End With

    With Range(Cells(PageRowOffset(Page) + BoxRowOffset(Box) + 4, 1 + BoxColOffset(Box)), Cells(PageRowOffset(Page) + BoxRowOffset(Box) + 7, 1 + BoxColOffset(Box) + 1)).Font
        .Name = "Calibri"
        .Size = 7
        .Strikethrough = False
        .Superscript = False
        .Subscript = False
        .OutlineFont = False
        .Shadow = False
        .Underline = xlUnderlineStyleNone
        .TintAndShade = 0
        .ThemeFont = xlThemeFontMinor
        .Bold = False
    End With

    Range(Cells(1 + PageRowOffset(Page) + BoxRowOffset(Box) + 1, 1 + BoxColOffset(Box) + 1), Cells(1 + PageRowOffset(Page) + BoxRowOffset(Box) + 2, 1 + BoxColOffset(Box) + 1)).Select
    Range(Cells(1 + PageRowOffset(Page) + BoxRowOffset(Box) + 1, 1 + BoxColOffset(Box) + 1), Cells(1 + PageRowOffset(Page) + BoxRowOffset(Box) + 2, 1 + BoxColOffset(Box) + 1)).NumberFormat = "#,##0.00"
    With Selection
        .HorizontalAlignment = xlGeneral
        .VerticalAlignment = xlTop
        .WrapText = False
        .Orientation = 0
        .AddIndent = False
        .IndentLevel = 0
        .ShrinkToFit = False
        .ReadingOrder = xlContext
        .MergeCells = False
    End With
End Sub

**How do you attack something like this?**
Sheets("report").Activate
       If fcnHasImage(Cells(15 + i, 24)) Then
            ActiveSheet.Cells(15 + i, 24).CopyPicture
Else
            ActiveSheet.Cells(15 + i, 2).CopyPicture         
     End If
       Sheets("A4").Select  '< - How should I this be changed?
       Cells(1 + PageRowOffset(Page) + BoxRowOffset(Box) + 7, BoxColOffset(Box) + 1).Select '< - This I guess is by changing it to Range?/Henrik
       ActiveSheet.Paste
       Application.CutCopyMode = False
       ShowProgress 'Run macro
       ActiveSheet.Cells(1, 25).Value = 15 + i + 
  End If...
Hankman3000
  • 115
  • 10
  • 1
    Have a look at the link: http://stackoverflow.com/questions/10714251/how-to-avoid-using-select-in-excel-vba-macros?rq=1 – Darren Bartrup-Cook Oct 13 '16 at 11:45
  • 2
    Why don't you just do what you did with the second and third `With` block with the first and last? – arcadeprecinct Oct 13 '16 at 11:46
  • @DarrenBartrup-Cook thanks but I seen all those links, the reason I still asking now is the fact that I cannot see how to relate them to my scenario. I'm not exactly a "VBA-prophet" as you probably can understand ; ) – Hankman3000 Oct 13 '16 at 11:50
  • @arcadeprecinct I take an extra look on that, thanks for the advice! – Hankman3000 Oct 13 '16 at 11:51
  • Quick advice, a few of the options that you are setting within your `With` block are default values. You don't need to set default values as by definition they are default and will be those even if you don't set them. It just adds unnecessary code which at times can make code difficult to read – Zac Oct 13 '16 at 12:11
  • Possible duplicate of [How to avoid using Select in Excel VBA](https://stackoverflow.com/questions/10714251/how-to-avoid-using-select-in-excel-vba) – BruceWayne Aug 21 '18 at 20:48

3 Answers3

2

The below is a shortened-up version of your code:

Sub FormatText()

    With Sheets("A4").Cells(1 + PageRowOffset(Page) + BoxRowOffset(Box) - 2, BoxColOffset(Box)).Font
        .Name = "Calibri"
        .Size = 11
        .Underline = xlUnderlineStyleNone
        .ThemeColor = xlThemeColorLight1
        .ThemeFont = xlThemeFontMinor
    End With



    With Range(Cells(PageRowOffset(Page) + BoxRowOffset(Box), 1 + BoxColOffset(Box)), Cells(PageRowOffset(Page) + BoxRowOffset(Box) + 3, 1 + BoxColOffset(Box) + 1)).Font
        .Name = "Calibri"
        .Size = 8
        .Underline = xlUnderlineStyleNone
        .ThemeFont = xlThemeFontMinor
    End With

    With Range(Cells(PageRowOffset(Page) + BoxRowOffset(Box) + 4, 1 + BoxColOffset(Box)), Cells(PageRowOffset(Page) + BoxRowOffset(Box) + 7, 1 + BoxColOffset(Box) + 1)).Font
        .Name = "Calibri"
        .Size = 7
        .Underline = xlUnderlineStyleNone
        .ThemeFont = xlThemeFontMinor
    End With

    Range(Cells(1 + PageRowOffset(Page) + BoxRowOffset(Box) + 1, 1 + BoxColOffset(Box) + 1), Cells(1 + PageRowOffset(Page) + BoxRowOffset(Box) + 2, 1 + BoxColOffset(Box) + 1)).NumberFormat = "#,##0.00"
    With Range(Cells(1 + PageRowOffset(Page) + BoxRowOffset(Box) + 1, 1 + BoxColOffset(Box) + 1), Cells(1 + PageRowOffset(Page) + BoxRowOffset(Box) + 2, 1 + BoxColOffset(Box) + 1))
        .HorizontalAlignment = xlGeneral
        .VerticalAlignment = xlTop
        .ReadingOrder = xlContext
    End With
End Sub
Jeremy
  • 1,337
  • 3
  • 12
  • 26
  • I accidentally edited your post. Can you see if you can roll it back. Sorry about that. –  Oct 14 '16 at 07:59
  • @ThomasInzina It's too late now - the green tick is gone! (only joking, no worries) – Jeremy Oct 14 '16 at 08:26
1

Select and Activate are basically just methods that are used in recording macros. To trim down a macro from there, you can do the following:

  • Anywhere ActiveCell is used, simply replace it with the Range reference that .Activate was called on. (In your case, the first With ActiveCell.Font would become With Sheets("A4").Cells(1 + PageRowOffset(Page) + BoxRowOffset(Box) - 2, BoxColOffset(Box)).Font)
  • Anywhere Selection is used, simply replace it with the Range reference that .Select was called on. (In your case, With Selection would become With Range(Cells(1 + PageRowOffset(Page) + BoxRowOffset(Box) + 1, 1 + BoxColOffset(Box) + 1), Cells(1 + PageRowOffset(Page) + BoxRowOffset(Box) + 2, 1 + BoxColOffset(Box) + 1)))

As an aside, when you correct that last With Selection block, you'll be able to move the .NumberFormat adjustment into the With block as well.

Some additional advice would be to get in the habit of establishing Worksheet objects that you can store the specific sheets your working in. So I would do something like Dim currentSheet As Worksheet and then somewhere before this block of code you've posted (where appropriate) Set currentSheet = Sheets("A4"). You'll have to update any Range(...) and Cells(...) calls to be currentSheet.Range(...), but the advantage of this is that your Range and Cells calls will always reference Sheets("A4") -- they won't accidentally switch context if you make modifications to this macro later on. This is how you also avoid relying on ActiveSheet, in general.

klords
  • 49
  • 2
1

Whenever you have to scroll horizontally to read your code; consider refactoring.

If you have a Range reference that contains two Cell references that share variables it would probably be better to use Range Resize.

Both of these examples are referring to the same Range. Using Range Resize we are able to remove shared variable.

Range(Cells(a + b, c), Cells(a + b + 10, c + 10))

Cells(a + b, c).Resize(10 + 1, 10 + 1)

Note: You will have to add one to the Columns and Rows parameter.

Option Explicit

Sub FormatText()
    Dim bc As Long, br As Long, pr As Long

    bc = BoxColOffset(Box)
    br = BoxRowOffset(Box)
    pr = PageRowOffset(Page)

    With Worksheets("A4")
        With .Cells(1 + pr + br - 2, bc).Font
            .Name = "Calibri"
            .Size = 11
            .Underline = xlUnderlineStyleNone
            .ThemeColor = xlThemeColorLight1
            .TintAndShade = 0
            .ThemeFont = xlThemeFontMinor
            .Bold = False
        End With
    End With

    With Worksheets("Sheet1")
        With .Cells(pr + br, 1 + bc).Resize(4, 2).Font
            .Name = "Calibri"
            .Size = 8
            .Strikethrough = False
            .Superscript = False
            .Subscript = False
            .OutlineFont = False
            .Shadow = False
            .Underline = xlUnderlineStyleNone
            .TintAndShade = 0
            .ThemeFont = xlThemeFontMinor
            .Bold = False
        End With

        With .Cells(pr + br + 4, 1 + bc).Resize(4, 2).Font
            .Name = "Calibri"
            .Size = 7
            .Strikethrough = False
            .Superscript = False
            .Subscript = False
            .OutlineFont = False
            .Shadow = False
            .Underline = xlUnderlineStyleNone
            .TintAndShade = 0
            .ThemeFont = xlThemeFontMinor
            .Bold = False
        End With

        With .Cells(1 + pr + br + 1, 1 + bc + 1).Resize(2)
            .NumberFormat = "#,##0.00"
            .HorizontalAlignment = xlGeneral
            .VerticalAlignment = xlTop
            .WrapText = False
            .Orientation = 0
            .AddIndent = False
            .IndentLevel = 0
            .ShrinkToFit = False
            .ReadingOrder = xlContext
            .MergeCells = False
        End With
    End With


    'Updated to answer:'**How do you attack something like this?**
    With Worksheets("report")
        If fcnHasImage(.Cells(15 + i, 24)) Then
            .Cells(15 + i, 24).CopyPicture
        Else
            .Cells(15 + i, 2).CopyPicture
        End If

        Sheets("A4").Cells(1 + pr + br + 7, bc + 1).PasteSpecial

        ShowProgress    'Run macro
        .Cells(1, 25).Value = 15 + i
    End With

End Sub
  • I was wondering about the code under **How do you attack something like this?**, please see original (edited) post. – Hankman3000 Oct 14 '16 at 07:45
  • I updated my answer. You need to use `Range().PasteSpecial` to paste the image of the range. If you can reference the image in the range it will have a CopyPicture method also. –  Oct 14 '16 at 08:01