Improving my macro skills - could you comment on this coding?

TheRedCardinal

Board Regular
Joined
Jul 11, 2019
Messages
149
I am developing my macro skills now and it's coming along well. I am developing a sheet that removes stacks of manual work from my team's tasks.

I have completed 2 phases of 7, but I worry that the process is not the most efficient.

So in this example, the sheet fills in a whole series of columns based on either data from a table, or from other sources (ranges in the workbook, or simple IF statements based on other values in the table).

Would you be so kind as to have a look at it and comment on whether I have made this more complex and longwinded than I needed to?

Code:
Sub GermanyCSV()


Dim PasteRange As Range
Dim NoTC As Range, Region As Range, Direction As Range, MoT As Range


Set WS1 = Sheets("5. Final CSV")
Set WS2 = Sheets("4. PivotTable")
Set WS3 = Sheets("Data Sheet")


Set PasteRange = WS1.Range("A1")
    If Range("RepDirection") = "Arrivals" Then
        Range("DEAFields").Copy
        Else
        Range("DEDFields").Copy
    PasteRange.PasteSpecial Paste:=xlPasteValues
    Application.CutCopyMode = False
    End If
    
Call LookupCommon


Counter = WS1.Range("F1").End(xlDown).Row


With WS1
    For Each CellA In Range(.Cells(2, 1), .Cells(Counter, 1))
        CellA.Offset(0, 1) = Range("RepMonth")
        
    
        If CellA.Offset(0, 6) = "GB" Then
            CellA.Offset(0, 3) = "1"
        Else
            CellA.Offset(0, 3) = "3"
        End If
            
        If CellA.Offset(0, 11) < 0 Then
            CellA.Offset(0, 2) = "21"
            CellA.Offset(0, 11) = -Round(CellA.Offset(0, 11), 0)
            CellA.Offset(0, 13) = -Round(CellA.Offset(0, 13), 0)
        Else
            CellA.Offset(0, 2) = "11"
            CellA.Offset(0, 11) = Round(CellA.Offset(0, 11), 0)
            CellA.Offset(0, 13) = Round(CellA.Offset(0, 13), 0)
        
        End If
        
        CellA.Offset(0, 14) = CellA.Offset(0, 13)
        
        CellA.Offset(0, 6).NumberFormat = "@"
        CellA.Offset(0, 7).NumberFormat = "@"
        
        If Range("RepDirection") = "Arrivals" Then
            CellA.Value = "E"
            CellA.Offset(0, 6) = "05"
            CellA.Offset(0, 8) = CellA.Offset(0, 6)
        Else
            CellA.Value = "V"
            CellA.Offset(0, 7) = "05"
        End If
    
    Next CellA
End With
            
End Sub

For example I feel like there must be a better way to code something like this:

Code:
    For Each WS1 In Application.ActiveWorkbook.Worksheets
        If WS1.Name <> "Intrastat - Start" And WS1.Name <> "1. Raw Data" And WS1.Name <> "2. Final Data" And WS1.Name <> "Data Sheet" And WS1.Name <> "Checklist" _
        And WS1.Name <> "3. Data Reconciliation" And WS1.Name <> "4. Pivot Table" Then
            WS1.Delete
        End If
    Next WS1

Thanks for any comments!
 

Some videos you may like

Excel Facts

Can a formula spear through sheets?
Use =SUM(January:December!E7) to sum E7 on all of the sheets from January through December

baitmaster

Well-known Member
Joined
Mar 12, 2009
Messages
2,039
I wouldn't use copy / paste unless I absolutely had to. I'd either make the destination range.value = the source range.value, or pass the values to an array and then pass the array to the destination range.value

Talking of arrays, your code has the potential to be slow because you change the worksheet many times. You'd be better off writing your calculations into an Array and then passing that array to the worksheet in one go. Working in VBA is tremendously fast, but interacting with Excel is comparatively slow, so avoid doing it. Here is an example of an array being created and then passed to Excel - there's a lot to know but these are some basics
Code:
Sub exampleArray()Dim i As Integer
Dim arr()                                           ' define an array


For i = 1 To 10                                     ' an example looping process
    ReDim Preserve arr(1 To 1, 1 To i)              ' expand your array. Use Preserve to retain existing contents. Can only increase last dimension when using preserve - hence we need to transpose later
    arr(1, i) = i                                   ' add data to new location in array
Next i


With Sheet1.Range("A1")
    .Resize(UBound(arr, 2) - LBound(arr, 2) + 1, UBound(arr, 1) - LBound(arr, 1) + 1).Value = Application.Transpose(arr)    ' set the destination value = the transposed array
End With


End Sub

I wouldn't work with the worksheet.name either, I'd use the worksheet.codename instead, since (1) worksheet.name can be easily changed within Excel and (2) anyone that knows how to change worksheet.codename knows not to unless the code is thoroughly checked too. Codename allows you to change the way that VBA sees the worksheet name, so you can standardise them in your own special way, e.g. renaming your key sheets to shtXXX, so then you only have to check IF INSTR(WS1.codename, "sht") > 0 THEN...
 

TheRedCardinal

Board Regular
Joined
Jul 11, 2019
Messages
149
Thanks, I'll take a look at those suggestions.
Understanding arrays is next on my list of things to grapple with!
 

Watch MrExcel Video

Forum statistics

Threads
1,123,346
Messages
5,601,087
Members
414,426
Latest member
fraru

We've detected that you are using an adblocker.

We have a great community of people providing Excel help here, but the hosting costs are enormous. You can help keep this site running by allowing ads on MrExcel.com.
Allow Ads at MrExcel

Which adblocker are you using?

Disable AdBlock

Follow these easy steps to disable AdBlock

1)Click on the icon in the browser’s toolbar.
2)Click on the icon in the browser’s toolbar.
2)Click on the "Pause on this site" option.
Go back

Disable AdBlock Plus

Follow these easy steps to disable AdBlock Plus

1)Click on the icon in the browser’s toolbar.
2)Click on the toggle to disable it for "mrexcel.com".
Go back

Disable uBlock Origin

Follow these easy steps to disable uBlock Origin

1)Click on the icon in the browser’s toolbar.
2)Click on the "Power" button.
3)Click on the "Refresh" button.
Go back

Disable uBlock

Follow these easy steps to disable uBlock

1)Click on the icon in the browser’s toolbar.
2)Click on the "Power" button.
3)Click on the "Refresh" button.
Go back
Top