Suspiciously redundant Excel macro code

Question

I need a little help in finding a better solution. In the screen shot below any change to the forecast line I need to find the value in column B. The screen shot is only a sample in my production sheet the row count is currently 4200.

           Column A  Col B   Col C   Col D  Col E 
Row 5              January
Row 6      Prg Desc   SD      FG      GH      DF
Row 7      Forecast    1       2       3       4
Row 8      Actual
Row 9      Compare

My current code does what I need it to, however, I'm sure there is a better method of accomplish my goal. In my production sheet the value in B is in row 5.

CurMonth = target.Offset(5 - target.Row).Value
        If CurMonth = "" Then
                CurMonth = target.Offset((5 - target.Row), -1).Value
            If CurMonth = "" Then
                CurMonth = target.Offset((5 - target.Row), -2).Value
            If CurMonth = "" Then
                CurMonth = target.Offset((5 - target.Row), -3).Value
            End If
            End If

        End If
    Debug.Print CurMonth

Any suggestions would be very appreciated.

Answer 1

Not relevant to your algo, but you shouldn't be using "" like this; use the vbNullString language constant instead, which consumes 0 memory (as opposed to "" which has the overhead of a string).

As for suggestions, in order to find a better way to do something, it's important to first understand what's going on. And proper indentation greatly helps for that:

CurMonth = target.Offset(5 - target.Row).Value
If CurMonth = vbNullString Then
    CurMonth = target.Offset((5 - target.Row), -1).Value
    If CurMonth = vbNullString Then
        CurMonth = target.Offset((5 - target.Row), -2).Value
        If CurMonth = vbNullString Then
            CurMonth = target.Offset((5 - target.Row), -3).Value
        End If
    End If
End If
Debug.Print CurMonth

Something sticks out right away - every new nested level seems to increment the offset, so what you really have is this:

CurMonth = target.Offset((5 - target.Row), n).Value
If CurMonth = vbNullString Then
    n = n - 1
    [and do that again while n > -3]

This looks like a recursive logic - something like this:

Private Function GetCurMonth(ByVal n As Long) As String
    Dim result As String

    result = target.Offset((5 - target.Row), n).Value
    If result = vbNullString And n > -3 Then
        n = n - 1
        result = GetCurMonth(n) 'recursive call
    End If

    GetCurMonth = result
End Sub

Also CurMonth has no reason not to be called CurrentMonth I guess. As Comintern mentioned, it would be easier to give a better review with more [code] context.

Answer 2

OK, I'll make a couple of assumptions here. First is that you are mainly looking for changes to individual cells or only changes in a specific row. The reason I am making that assumption is that if it isn't true, you aren't handling the cases where changes occur in multiple months at the same time. Remember, the Target object that you get as a parameter to Worksheet_Change is a Range - not a Row or a Cell. When you call Target.Row, it returns the top row in the passed Range making no guarantees that only 1 Row was altered. A good way to check to see if your final code handles this robustly is to select a 5x5 or 10x10 array of cells and clear them. Does this give you the desire behavior? If not, you may have to re-think your approach to the problem.

Now, going on the assumption that you only care about single cell or single row edits, you need to test for that in your code. My general rule is to always try to resolve a Range into something more useful. The .Offset method is typically asking for trouble, as it gives a value that is relative to the Range as opposed to having a hard location on the Worksheet. You can always get your location on the Worksheet from a Range by getting the Row and Column of the upper-left most Cell:

Private Sub Worksheet_Change(ByVal Target As Range)

    Debug.Print Target.Cells(1, 1).Row
    Debug.Print Target.Cells(1, 1).Column

End Sub

Once you have an absolute Row and Column relative to the Worksheet, it makes your calculations a lot simpler. For example: you can test to see if the Row you returned if one that you care about, and then use that knowledge to directly address the other cells you care about without fumbling around with the Offset.

Private Sub Worksheet_Change(ByVal Target As Range)

    Dim CurrentRow As Integer
    CurrentRow = Target.Cells(1, 1).Row

    If Me.Cells(CurrentRow, 1).Value = "Forecast" Then
        'In the correct row.
        CurrentMonth = Me.Cells(CurrentRow - 2, 2)
    Else
        'Must not have been on the Forecast row.
    End If

End Sub