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...
Improve this question
6
  • 1
    Have a look at the link: stackoverflow.com/questions/10714251/… Commented Oct 13, 2016 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? Commented Oct 13, 2016 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 ; ) Commented Oct 13, 2016 at 11:50
  • @arcadeprecinct I take an extra look on that, thanks for the advice! Commented Oct 13, 2016 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 Commented Oct 13, 2016 at 12:11

3 Answers 3

Reset to default
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
Improve this answer
Sign up to request clarification or add additional context in comments.

2 Comments

I accidentally edited your post. Can you see if you can roll it back. Sorry about that.
@ThomasInzina It's too late now - the green tick is gone! (only joking, no worries)
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.

Improve this answer

1 Comment

any clue on my second question?
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
Improve this answer

2 Comments

I was wondering about the code under How do you attack something like this?, please see original (edited) post.
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.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.