# Suspiciously redundant Excel macro code

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.

-
+1 but I hardly call that a screenshot ;) – Mat's Mug Feb 14 '14 at 4:42
It isn't exactly clear what you are trying to accomplish here. Is the code you posted in a Worksheet event, a custom defined function, a macro, or something else? Can you post some more of the code to give more context? – Comintern Feb 14 '14 at 4:46
Thought that was an awesome screenshot! The code is part of a worksheet change event. The desire was when a user changes the value in the forecast line it will return the value in B5. – User406160 Feb 14 '14 at 4:56
Is the month in a merged cell? It's hard to tell from the "screenshot". – Comintern Feb 14 '14 at 5:10
No the month is centered across the selection of row 5 columns b, c, d & e – User406160 Feb 14 '14 at 5:14

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.

-
Can't you write it with a WHILE instead of recursion? – abuzittin gillifirca Feb 14 '14 at 9:28
@abuzittin probably. That was just a quickie :) – Mat's Mug Feb 14 '14 at 16:55

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

-