Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

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.

share|improve this question
2  
+1 but I hardly call that a screenshot ;) –  Mat's Mug Feb 14 at 4:42
3  
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 at 4:46
1  
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 at 4:56
    
Is the month in a merged cell? It's hard to tell from the "screenshot". –  Comintern Feb 14 at 5:10
    
No the month is centered across the selection of row 5 columns b, c, d & e –  User406160 Feb 14 at 5:14
show 1 more comment

2 Answers

up vote 5 down vote accepted

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.

share|improve this answer
1  
Can't you write it with a WHILE instead of recursion? –  abuzittin gillifirca Feb 14 at 9:28
    
@abuzittin probably. That was just a quickie :) –  Mat's Mug Feb 14 at 16:55
add comment

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
share|improve this answer
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.