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

[TheRedCardinal]

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?

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:

    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!

 

[baitmaster]

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

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]

Thanks, I'll take a look at those suggestions.
Understanding arrays is next on my list of things to grapple with!