I started with VBA yesterday. Could this code be improved?

Private Sub Worksheet_Change(ByVal target As Range)
    If target.Column = 3 Then
        Call Modul1.AutoFill(target)
    End If
End Sub

Option Explicit

Sub AutoFill(activeCell As Range)
    Dim storedData As Range: Set storedData = GrabStoredData(activeCell.Value)
    If Not storedData Is Nothing Then
        Call FillRowWith(activeCell, storedData)
    End If
End Sub

Private Function GrabStoredData(itemName As String) As Range
    Dim itemCell As Range
    Set itemCell = Sheets("SourceSheet").Range("A:A").Find(what:=itemName, lookat:=xlWhole)

    If Not itemCell Is Nothing Then
        Set GrabStoredData = Sheets("SourceSheet").Range("B" & itemCell.row & ":G" & itemCell.row)
    End If
End Function

Private Sub FillRowWith(activeCell As Range, storedData As Range)
    Dim counter As Integer
    For counter = 1 To 6
        Sheets("TargetSheet").Cells(activeCell.row, activeCell.Column + counter).Value = storedData.Offset(0, counter - 1).Value
    Next counter
End Sub

We have a SourceSheet that works as the "Database". The SourceSheet has 7 columns, with the first one used to identify an item or order. The other 6 columns contain information.

Lets say we put an order in "TargetSheet" which already exists in "SourceSheet". It will find the order in "SourceSheet" and take the information in the 6 columns and replace 6 columns in "TargetSheet" with that information.

This demo should show it better.




People read code downwards much better than they read across. Almost all code everywhere operates under the convention "(at most) 1 statement/operation per line". Colons : are small and very easily missed. Just stick subsequent operations on newlines and you'll avoid a lot of confusion/errors down the line.

Call is fully deprecated

It's not needed anymore and your code will run just fine without it. Assuming you name your methods well, dropping Call will also means your code reads better.

You will need to modify your calling syntax slightly though. When you have a DoThing Sub, this is valid syntax:

Call DoThing(Arg1, Arg2)

But this is not:

DoThing(Arg1, Arg2)

Functions get brackets. Subs do not. Like so:

Var = Function(Arg1, Arg2)
DoThing Arg1, Arg2

Don't let renaming break your code

Call Modul1.AutoFill(target)

Sheets("SourceSheet").Range("B" & itemCell.row & ":G" & itemCell.row)

If that module gets renamed, or that sheet gets renamed, your code will break.

Fortunately there are easy solutions.

1: Don't qualify your module references

You shouldn't need to specify which module your subs are in. If you do, this implies wider problems with your code. If your methods are descriptively named and you don't do something horrible like having 2 identically-named methods in different modules, just call your method using its signature.

2: Sheets have Codenames

If you go to the properties window and select a Worksheet, there will be a (name) property. This is the sheet's Codename. By default, new sheets have codenames like Sheet1, Sheet2 etc. A Codename is effectively a global worksheet variable that always refers to that sheet.

So, assuming you give "SourceSheet" a Codename of sourceSheet then your code can simply be:

sourceSheet.Range("B" & itemCell.row & ":G" & itemCell.row)

And now your users can name the sheets whatever they like and your code will still run fine.

Be Explicit with your naming

Your method names are OK, but very, very ambiguous. AutoFill, GrabStoredData, FillRowWith, they all imply a level of generality that is at odds with what they actually do.

As a rule, names should be descriptive, then unambiguous and only then concise. Easy-To-Understand trumps Shorter-Name every time.

GrabStoredData(itemName). Not a clue what that does in particular.

GetRowRangeOfItemData(itemName). I immediately know exactly what that does. It's going to return a Range Object for a given item. This Range Object will be on a single row.

AutoFill(Cell). Fill What? Where? With what?

FillItemDetailsToRightOfCell(Cell). Takes about 0.2 seconds longer to read. Takes much less time to understand. Especially when someone else (or you in 6 months time) is coming back to this code with no memory of the internal workings.

FillRowWith(Range, Cell). If I see that I'm going to assume it, well, fills a row. No Idea what those variables are, particularly.

PasteRangeToRightOfCell(Range, Cell). Again, takes a fraction of a second longer to read, takes no time to understand exactly what's going on.

  • \$\begingroup\$ Thank you. I've made the necessary changes. Just one thing. I still have to use "Call" or it gives me an error. \$\endgroup\$ – Anonymous Jul 6 '16 at 17:17
  • \$\begingroup\$ @Robiow Ah yes. See edits. \$\endgroup\$ – Kaz Jul 6 '16 at 18:54

Whenever you change something on the Worksheet Object from within a Worksheet_Change event macro (or any process called from within it) always disable the Application.EnableEvents property. Failure to temporarily halt event handling will try to get the Worksheet_Change to run on top of itself after being triggered by the new change on the worksheet.

Additionally, you should not turn off event handling without error control that will restore it if things go south.

Target can be a single cell or many cells (as in a paste operation to a group of cells. Typically, the Intersect method is used to see if there is an overlap between the user's are of concern and whatever cell or cells that Target encompasses.

Naming your sub procedure the same as the Range.AutoFill method is not considered 'best practise'. Consider changing it to something like myAutoFill.

You seem to want a single cell for the parameter so I've cycled through all matching cells in the intersection of column C and Target in case there is more than a single cell. I'll try have a look later to see if a bulk operation is possible.

Private Sub Worksheet_Change(ByVal Target As Range)
    If not intersect(target, range("C:C")) is nothing Then
        on error goto bm_Safe_Exit:
        application.enableevents = false
        dim trgt as range
        for each trgt in intersect(target, range("C:C"))
            'should there be a check here to see if the value was simply deleted before proceeding?
            Modul1.AutoFill trgt     '<~~ should this be Module1 ?
    End If
    application.enableevents = true
End Sub
  • \$\begingroup\$ I've had another look at the companion sub procedures and in their current state, processing multiple matching cells one-by-one is the better method. However, using activeCell as the parameter's name (in two places) can only lead to confusion with the ActiveCell property which is generally not target since a typed value typically moves off the cell that has been changed once [enter] has been pressed. \$\endgroup\$ – user66882 Jul 8 '16 at 22:01

Also consider if your data for the target row is the same shape as the source row then i believe you can use the Value2 property. Assigning Value2 is quicker than iterating through each cell and assigning the Value property.

targetRow.Value2 = sourceRow.Value2

You can also use Value2 if your data is an array in the correct shape.


Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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