Cleaning up ExcelVBA

Question

I'm new with VBA for excel and asking for your expertise.

I made a recording Marco witch works totaly fine, the problem is that I know it can be shorter and look more nicer, and maybe go even faster to run.

I've read that the .Select shall be avoided as much as possible, and when recording Macros, it does this automatically.

Sub Audit_chat()

Range("R13").Select
Selection.Copy
Range("F2:K2").Select
Range(Selection, Selection.End(xlDown)).Select
Selection.PasteSpecial Paste:=xlPasteValues, Operation:=xlAdd, SkipBlanks _
    :=False, Transpose:=False
Application.CutCopyMode = False
Selection.NumberFormat = "[h]:mm:ss"
Columns("F:K").Select
Selection.Replace What:="No Value", Replacement:="0", LookAt:=xlPart, _
    SearchOrder:=xlByRows, MatchCase:=False, SearchFormat:=False, _
    ReplaceFormat:=False
Range("B:B,C:C,N:N,O:O").Select
Range("O1").Activate
Selection.Copy
Sheets("Agents").Select
Range("A1").Select
ActiveSheet.Paste
Application.CutCopyMode = False
ActiveSheet.Range("$A$1:$D$1048575").RemoveDuplicates Columns:=Array(1, 2), _
    Header:=xlYes
Columns("D:D").Select
Selection.Copy
Range("C1").Select
ActiveSheet.Paste
Sheets("Counter").Select
Range("A1").Select

End Sub

Can this be fixed, or am I "doomed" for life? :)

Explaination of what it does.

Range("R13").Select
    Selection.Copy 

'' Copy a blank cell

Range("F2:K2").Select
Range(Selection, Selection.End(xlDown)).Select
Selection.PasteSpecial Paste:=xlPasteValues, Operation:=xlAdd, SkipBlanks _
    :=False, Transpose:=False
Application.CutCopyMode = False

'' Select Range F2:K2 all the way to the end of the columns    

Selection.NumberFormat = "[h]:mm:ss"

'' set the numbers to [h]:mm:ss

Reason: The file I has have the cells in the wrong format, and even if I change the format, It will not update, but I found out that If I copied a blank cell over it as a special paste with "Value" and "Add" it fixed the problem.

   Columns("F:K").Select
    Selection.Replace What:="No Value", Replacement:="0", LookAt:=xlPart, _
        SearchOrder:=xlByRows, MatchCase:=False, SearchFormat:=False, _
        ReplaceFormat:=False

'' In Colums F:K find and replace "No Value" (Text) to "0"

   Range("B:B,C:C,N:N,O:O").Select
    Range("O1").Activate
    Selection.Copy
    Sheets("Agents").Select
    Range("A1").Select
    ActiveSheet.Paste
    Application.CutCopyMode = False

'' Copy all data in B:B,C:C,N:N,O:O, and paste it in Sheet "Agents"

   ActiveSheet.Range("$A$1:$D$1048575").RemoveDuplicates Columns:=Array(1, 2), _
        Header:=xlYes

'' Remove duplicates in all cells A:D and has a header

   Columns("D:D").Select
    Selection.Copy
    Range("C1").Select
    ActiveSheet.Paste

'' Copy the all the information from colum D and paste it in C

Sheets("Counter").Select
Range("A1").Select

'' Go to Sheet "Counter"

Thanks in advance.

Best Regards, Peter

This question should really be on codereview, however take a look at how to avoid `.Select` http://stackoverflow.com/questions/10714251/how-to-avoid-using-select-in-excel-vba-macros — Tim Wilkinson, Mar 10 '17 at 14:12
Cleaning up ExcelVBA is a very misleading topic. Could you please change this? — ruedi, Mar 10 '17 at 14:37

score 1 · Answer 1 · answered Mar 10 '17 at 16:42

Writing code like the macro recorder will be a nightmare to maintain.

Here's my attempt at a cleanup (Far, far from perfect)(untested);

Sub x()

    '///////////////////
    '// First Action //
    '/////////////////
    Range("R13").Select
    Selection.Copy
    Range("F2:K2").Select
    Range(Selection, Selection.End(xlDown)).Select
    Selection.PasteSpecial Paste:=xlPasteValues, Operation:=xlAdd, SkipBlanks _
        :=False, Transpose:=False
    Application.CutCopyMode = False
    Selection.NumberFormat = "[h]:mm:ss"

    '// Try //
    Sheets("MySheet").[F2:K2].Value = [R13].Value
    Sheets("MySheet").[F2:K2].NumberFormat = "[h]:mm:ss"

    '////////////////////
    '// Second Action //
    '//////////////////
    Columns("F:K").Select
    Selection.Replace What:="No Value", Replacement:="0", LookAt:=xlPart, _
        SearchOrder:=xlByRows, MatchCase:=False, SearchFormat:=False, _
        ReplaceFormat:=False

    '// Try //
    Sheets("MySheet").[F:K].Replace What:="No Value", Replacement:="0", LookAt:=xlPart

    '///////////////////
    '// Third Action //
    '/////////////////

    Range("B:B,C:C,N:N,O:O").Select
    Range("O1").Activate
    Selection.Copy
    Sheets("Agents").Select
    Range("A1").Select
    ActiveSheet.Paste
    Application.CutCopyMode = False
    ActiveSheet.Range("$A$1:$D$1048575").RemoveDuplicates Columns:=Array(1, 2), _
        Header:=xlYes

    '// Try //
    Sheets("MySheet").Range("B:B,C:C,N:N,O:O").Copy Sheets("Agents").[A1]
    Sheets("Agents").[A:D].RemoveDuplicates Columns:=Array(1, 2), Header:=xlYes

    '////////////////////
    '// Fourth Action //
    '////////////////////

    Columns("D:D").Select
    Selection.Copy
    Range("C1").Select
    ActiveSheet.Paste
    Sheets("Counter").Select
    Range("A1").Select ' I think this only exists to go back to where you started

    '// Try //
    Sheets("Mysheet").[D:D].Copy [C:C]

    '////////////////////////
    '// So, total code is //
    '//////////////////////

    Sheets("MySheet").[F2:K2].Value = [R13].Value
    Sheets("MySheet").[F2:K2].NumberFormat = "[h]:mm:ss"

    Sheets("MySheet").[F:K].Replace What:="No Value", Replacement:="0", LookAt:=xlPart

    Sheets("MySheet").Range("B:B,C:C,N:N,O:O").Copy Sheets("Agents").[A1]
    Sheets("Agents").[A:D].RemoveDuplicates Columns:=Array(1, 2), Header:=xlYes

    Sheets("Mysheet").[D:D].Copy [C:C]
End Sub

If you activate/select a cell/sheet to manipulate it, you're doing yourself a disservice, you should never need to*

* = Unless the macro/code is to specifically access a cell/sheet of interest (Like a "go to agents list sheet" button or something)

Brent · Answer 2 · 2017-03-10T16:46:07.527

Whew! That is some ugly code. When you record a macro the result isn't easy to read.

Can you tell me what you're trying to do? That will help me to clean-up your code.

".Activate" vs. ".Select"

Also here is the layman's explanation on the difference between "Activate" and "Select":

With ".Select", for example worksheets, you can have more than one worksheet selected. ".Select" allows you to conduct operations on multiple objects at one time.

With ".Activate", for example worksheets, only allows you to have one worksheets active at a time. So in the below code you will have three worksheets that are selected but only one activated.

Worksheets(Array("Sheet1", "Sheet2", "Sheet3")).Select
Worksheets("Sheet2").Activate

In the below code you will only have one worksheet selected.

Worksheets(Array("Sheet1", "Sheet2", "Sheet3")).Select
Worksheets("Sheet2").Select

The reason why ".Select" can get you in trouble is because if you select several objects you will conduct operations on all of the objects you select. You may or may not want that. Using ".Activate" limits your operations to only one object.

Solution 01

Below is the first attempt at a solution. In general I would recommend using the VBA objects and Excel objects to your advantage and comment the code well. Below is one option on how to do that.

The code is longer but it is clearer and much easier to understand while taking advantage of the VBA / Excel object library.

I have not tested the below code.

Sub Audit_chat()

    '''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''
    '''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''
    '
    ' variables / object declaration
    '
    '''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''
    '''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''

    ' declare objects
    Dim wks_dest As Worksheet, wks_source As Worksheet
    Dim rng_srce_copy_01 As Range, rng_dest_01 As Range, rng_srce_copy_02 As Range
    Dim rng_dest_dup_01 As Range, rng_srce_copy_03 As Range

    '''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''
    '''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''
    '
    ' variables / object initialzation
    '
    '''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''
    '''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''

    ' set worksheet objects
    ' I don't know the name of the source worksheet
    Set wks_source = Worksheets("<Source Worksheet Name>")
    Set wks_dest = Worksheets("Agents")

    ' set source range objects
    Set rng_srce_copy_01 = wks_source.Range("R13")
    Set rng_srce_copy_02 = wks_source.Range("O1")
    Set rng_srce_copy_03 = wks_dest.Range("D:D")

    ' set desstination range objects
    Set rng_dest_01 = wks_source.Range("F:K")
    Set rng_dest_dup_01 = wks_dest.Range("$A$1:$D$1048575")

    '''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''
    '''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''
    '
    ' start main method
    '
    '''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''
    '''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''

    ' copy the source 01
    rng_srce_copy_01.Copy

    ' paste information from range_srce_copy_01
    With rng_dest_01
        .PasteSpecial Paste:=xlPasteValues, _
                      Operation:=xlAdd, _
                      SkipBlanks:=False, _
                      Transpose:=False

        ' change cell format
        .NumberFormat = "[h]:mm:ss"

        ' replace "No Value" with 0
        .Replace What:="No Value", _
                 Replacement:="0", _
                 LookAt:=xlPart, _
                 SearchOrder:=xlByRows, _
                 MatchCase:=False, _
                 SearchFormat:=False, _
                 ReplaceFormat:=False
    End With

    ' application mode turn off
    Application.CutCopyMode = False

    ' copy source 02
    ' this will only copy one cell "O1" which is what your code is doing
    ' if you want to copy columns B, D, N, O then you need to define your
    ' range objct as:
    ' Set rng_srce_copy_02 = Range("B:B,C:C,N:N,O:O")
    ' this is where Select vs. Activate gets you in trouble
    ' do you want all the colums or just cell?
    rng_srce_copy_02.Copy

    ' go to destination worksheet
    ' you may have to break this up into:
    ' wks_dest.Activate
    ' Range("A1").Activate
    ' but I don't think so
    wks_dest.Range("A1").Activate
    wks_dest.Paste

    ' application mode turn off
    Application.CutCopyMode = False

    ' look at all the cells in the first two columns and remove
    ' the duplicates
    rng_dest_dup_01.RemoveDuplicates Columns:=Array(1, 2), _
                                     Header:=xlYes

    ' copy range 03
    rng_srce_copy_03.Copy

    ' paste at cell C1
    Range("C1").Select
    wks_dest.Paste

    ' go to "Counter" worksheet
    Worksheets("Counter").Activate
    Range("A1").Activate

End Sub

I really like Richard's solution, elegant and less lines of code. — Brent, Mar 10 '17 at 16:51

Vinicius B · Answer 3 · 2017-03-10T17:30:47.653

-1

you can try to "Join" the range("").select with the next line, for example

Range("R13").Select
Selection.Copy

Can be:

Range("R13").Copy

Try this:

Sub Audit_chat()

Range("R13").Copy
Range("F2:K2").Select
Range(Selection, Selection.End(xlDown)).Select
Selection.PasteSpecial Paste:=xlPasteValues, Operation:=xlAdd, SkipBlanks _
    :=False, Transpose:=False
Selection.NumberFormat = "[h]:mm:ss"
Columns("F:K").Replace What:="No Value", Replacement:="0", LookAt:=xlPart, _
    SearchOrder:=xlByRows, MatchCase:=False, SearchFormat:=False, _
    ReplaceFormat:=False
Range("O1").Copy
Sheets("Agents").Range("A1").Select
ActiveSheet.Paste
Application.CutCopyMode = False
ActiveSheet.Range("$A$1:$D$1048575").RemoveDuplicates Columns:=Array(1, 2), _
    Header:=xlYes
Columns("D:D").Copy
Range("C1").Select
ActiveSheet.Paste
Sheets("Counter").Range("A1").Select

End Sub

edited Mar 10 '17 at 17:30

answered Mar 10 '17 at 14:31

Vinicius B

162
9

1

You made a valid first point, then didn't heed your own advice in the example! – Tim Wilkinson Mar 10 '17 at 17:21
yes, I did'nt notice that I missed some lines, thanks! – Vinicius B Mar 10 '17 at 17:32
might want to edit then as otherwise this is a potentially useless answer. – Tim Wilkinson Mar 11 '17 at 23:28
If you are willing to help, there is no useless answer, of course there are answers that are more useful than others, but it can be an oportunity to learn ;) – Vinicius B Mar 13 '17 at 11:21

Cleaning up ExcelVBA

3 Answers3

".Activate" vs. ".Select"

Solution 01