1

I am not used to writing code. I normally generate my code via macro and I am facing this issue. Can someone please help me?

Sub Test()

    Dim WorkRng As Range
    Dim Rng As Range
    Dim xOffsetColumn As Integer

    Set WorkRng = Intersect(Application.ActiveSheet.Range("B8:B38"), Target)
    xOffsetColumn = 19

    If Not WorkRng Is Nothing Then
        Application.EnableEvents = False

        For Each Rng In WorkRng
            If Not VBA.IsEmpty(Rng.Value) Then
                Rng.Offset(0, xOffsetColumn).Value = Now
                Rng.Offset(0, xOffsetColumn).NumberFormat = "mm/dd/yyyy, hh:mm:ss"
            Else
                Rng.Offset(0, xOffsetColumn).ClearContents
            End If
        Next

        Application.EnableEvents = True
    End If

    Dim WorkRng1 As Range
    Dim Rng1 As Range
    Dim xOffsetColumn1 As Integer

    Set WorkRng1 = Intersect(Application.ActiveSheet.Range("C8:C38"), Target)
    xOffsetColumn1 = 18

    If Not WorkRng1 Is Nothing Then

        For Each Rng1 In WorkRng1
            If Not VBA.IsEmpty(Rng1.Value) Then
                Rng1.Offset(0, xOffsetColumn1).Value = Now
                Rng1.Offset(0, xOffsetColumn1).NumberFormat = "mm/dd/yyyy, hh:mm:ss"
            Else
                Rng1.Offset(0, xOffsetColumn1).ClearContents
            End If
        Next

        Application.EnableEvents = True
    End If

    ....................................
    ..............................

    Dim WorkRng132 As Range
    Dim Rng132 As Range
    Dim xOffsetColumn132 As Integer

    Set WorkRng132 = Intersect(Application.ActiveSheet.Range("EJ8:EJ38"), Target)
    xOffsetColumn132 = 1

    If Not WorkRng132 Is Nothing Then

        For Each Rng132 In WorkRng132
            If Not VBA.IsEmpty(Rng132.Value) Then
                Rng132.Offset(0, xOffsetColumn132).Value = Now
                Rng132.Offset(0, xOffsetColumn132).NumberFormat = "mm/dd/yyyy, hh:mm:ss"
            Else
                Rng132.Offset(0, xOffsetColumn132).ClearContents
            End If
        Next

        Application.EnableEvents = True
    End If

End Sub
dwirony
  • 5,487
  • 3
  • 21
  • 43
Hoang The Tai
  • 19
  • 1
  • 2
  • 1
    I can already tell that you could easily, in one loop, cycle through all those ranges (instead of having 132 blocks of the same code). – dwirony Aug 20 '18 at 16:03
  • 1
    Voted to re-open. The answers suggested in the linked question are a poor response to the OP's problem - splitting a verbose procedure into many sub-procedures just pushed the problem around a little but, and does nothing to address the root cause of the issue here. It would be more fruitful to point the OP toward the concept of refactoring and parameterised sub-procedures. – Tim Williams Aug 20 '18 at 16:06
  • Despite the link to the duplicate question.. do not split this up. Anytime you find yourself repeating code, write a function. The only thing that is changing between each iteration appears to be the range. A loop with this function would solve your issue. – JosephC Aug 20 '18 at 16:06
  • You can give yourself an example based on the other code, thansk – Hoang The Tai Aug 20 '18 at 16:10
  • Can you help me understand how `xOffsetColumn1` works? Is it just `1` for 100+ loops? – Marcucciboy2 Aug 20 '18 at 16:12
  • I have xOffsetColumn230 = 1 and xOffsetColumn230 = 19 – Hoang The Tai Aug 20 '18 at 16:13
  • Have you seen [THIS](https://stackoverflow.com/questions/11450232/getting-error-procedure-too-large-in-vba-macros-excel/11450945#11450945) – Siddharth Rout Aug 20 '18 at 16:17
  • I have already seen it but really do not understand to implement it – Hoang The Tai Aug 20 '18 at 16:28

1 Answers1

7

One useful maxim in programming is Don't Repeat Yourself (DRY) - duplicated code is longer, harder to understand, and difficult to maintain.

There's a clear repeating pattern in your code. This block:

Dim WorkRng As Range
Dim Rng As Range
Dim xOffsetColumn As Integer

Set WorkRng = Intersect(Application.ActiveSheet.Range("B8:B38"), Target)
xOffsetColumn = 19

If Not WorkRng Is Nothing Then
    Application.EnableEvents = False

    For Each Rng In WorkRng
        If Not VBA.IsEmpty(Rng.Value) Then
            Rng.Offset(0, xOffsetColumn).Value = Now
            Rng.Offset(0, xOffsetColumn).NumberFormat = "mm/dd/yyyy, hh:mm:ss"
        Else
            Rng.Offset(0, xOffsetColumn).ClearContents
        End If
    Next

    Application.EnableEvents = True
End If

Can be refactored into a re-usable method with two parameters:

Sub Test()
    '....
    ProcessRange Application.Intersect(Me.Range("B8:B38"), Target), 19
    ProcessRange Application.Intersect(Me.Range("C8:C38"), Target), 18
    'etc for the other ranges
    '....
End sub


'subprocedure
Sub ProcessRange(WorkRng As Range, offsetCol as Long)
    Dim Rng As Range
    If Not WorkRng Is Nothing Then
        Application.EnableEvents = False
        For Each Rng In WorkRng
            With Rng.Offset(0, offsetCol)
            If Not VBA.IsEmpty(Rng.Value) Then
                .Value = Now
                .NumberFormat = "mm/dd/yyyy, hh:mm:ss"
            Else
                .ClearContents
            End If
            End With
        Next
        Application.EnableEvents = True
    End If

End Sub
Tim Williams
  • 154,628
  • 8
  • 97
  • 125
  • Might I suggest a loop of say, 2 to 140 (B to EJ), where a UDF can get the letter value of the number, that way he doesn't need 140 lines of `ProcessRange`? Although I'm not sure how he's getting that column offset number... – dwirony Aug 20 '18 at 16:18
  • Nicely done.. I almost closed this post as a duplicate... :D – Siddharth Rout Aug 20 '18 at 16:19
  • I do not understand this paragraph'.... ProcessRange Application.Intersect(Me.Range("B8:B38"), Target), 19 ProcessRange Application.Intersect(Me.Range("C8:C38"), Target), 18 '.... 'subprocedure – Hoang The Tai Aug 20 '18 at 16:23
  • Those two lines are in your main procedure, and call `ProcessRange` passing two parameters, a range and a column number. – Tim Williams Aug 20 '18 at 16:44
  • @dwirony - I didn't spend a long time looking at the next level up: likely what you suggest would be the next step in refactoring, but without the full procedure it's difficult to be certain exactly what's going on. – Tim Williams Aug 20 '18 at 16:46