Copying cells from one sheet to another

Question

I'm new to writing VBA and having spent most of the day writing some code, I have finally got it working but it looks horrible. I'm hoping that someone can help me tidy it up.

The code simply copies certain cells from one sheet into another. If a particular cell in the source sheet contains the word "Accepted". Columns A of the source and destination sheets contain a unique reference and that data is only copied across if the unique reference is not already in the destination data. In addition, my code adds the date at which each entry is made in the destination sheet.

Ideally I need the code to look tidy and professional and probably most importantly be understandable to anyone looking at it, whether that be someone else or me in a years time - when I've forgotten what I did.

Public Sub UpdateAcceptedChangeRequests()

With Application
    .ScreenUpdating = False
    .EnableEvents = False
End With

On Error GoTo errorHandler:

Dim varAnswer As String
    varAnswer = MsgBox("Are you sure you want to update the Accepted Change Requests List?", vbYesNo, "Update Accepted Change Requests")
        If varAnswer = vbNo Then
            MsgBox ("No changes saved")
                With Application
                    .ScreenUpdating = True
                    .EnableEvents = True
                End With
            Exit Sub
        End If

Dim SourceRange As Range, DestRange As Range
Dim DestSheet As Worksheet, SourceSheet As Worksheet
Dim LastRowDestSheet As Long, i As Long, LastRowSourceSheet As Long
    Set DestSheet = ThisWorkbook.Worksheets("Accepted Change Requests")
    Set SourceSheet = ThisWorkbook.Worksheets("All Change Requests")

        LastRowDestSheet = DestSheet.Cells(DestSheet.Rows.Count, "A").End(xlUp).Row
        LastRowSourceSheet = SourceSheet.Cells(SourceSheet.Rows.Count, "E").End(xlUp).Row

            For i = 2 To LastRowSourceSheet
                If SourceSheet.Range("E" & i).Value = "Accepted" Then
                    If Evaluate("ISERROR(MATCH(A" & i & ",'Accepted Change Requests'!A:A,0))") Then
                        DestSheet.Range("A" & LastRowDestSheet + 1 & ":D" & LastRowDestSheet + 1).Value = _
                            SourceSheet.Range("A" & i & ":D" & i).Value
                            With DestSheet.Range("E" & LastRowDestSheet + 1)
                                .Value = Date
                                .NumberFormat = "dd/mm/yyyy"
                            End With
                        LastRowDestSheet = LastRowDestSheet + 1
                    End If
                End If
            Next i

        With Application
        .ScreenUpdating = True
        .EnableEvents = True
        End With

Exit Sub

errorHandler:
MsgBox ("There was an error adding this Change Request")
Resume Next

With Application
    .ScreenUpdating = True
    .EnableEvents = True
End With

End Sub

Answer 1

When you'll look at this code a number of years months (weeks?) from now, the first thing that will jump at you and make you want to rip your eyes off, is the indentation.

The idea is that code blocks add a level. What's a code block you'll ask? A Sub or a Function defines a scope - as such, the only thing you should see at the same indentation level as the words Public Sub, is the words End Sub - and labels, because the VBE wants them unindented.

Declaring a variable does not constitute a code block. These lines should all be at the same indentation level:

Dim SourceRange As Range, DestRange As Range
Dim DestSheet As Worksheet, SourceSheet As Worksheet
Dim LastRowDestSheet As Long, i As Long, LastRowSourceSheet As Long
    Set DestSheet = ThisWorkbook.Worksheets("Accepted Change Requests")
    Set SourceSheet = ThisWorkbook.Worksheets("All Change Requests")

        LastRowDestSheet = DestSheet.Cells(DestSheet.Rows.Count, "A").End(xlUp).Row
        LastRowSourceSheet = SourceSheet.Cells(SourceSheet.Rows.Count, "E").End(xlUp).Row

            For i = 2 To LastRowSourceSheet

Like this:

Dim SourceRange As Range, DestRange As Range
Dim DestSheet As Worksheet, SourceSheet As Worksheet
Dim LastRowDestSheet As Long, i As Long, LastRowSourceSheet As Long
Set DestSheet = ThisWorkbook.Worksheets("Accepted Change Requests")
Set SourceSheet = ThisWorkbook.Worksheets("All Change Requests")

LastRowDestSheet = DestSheet.Cells(DestSheet.Rows.Count, "A").End(xlUp).Row
LastRowSourceSheet = SourceSheet.Cells(SourceSheet.Rows.Count, "E").End(xlUp).Row

For i = 2 To LastRowSourceSheet

I would avoid declaring multiple variables in a single instruction. You've done it right though - every variable has a type and that's good (common beginner mistake is to only specify a type for the last one, making all but the last one Variant variables)... but things are simply easier to read (and to maintain!) when each declaration has its Dim statement.

A With block however, is a block, and as such warrants an indentation level. Same with For and If blocks, which you all got right.

---

The parentheses are not needed here, and can play bad tricks on you if you make it a habit:

MsgBox ("There was an error adding this Change Request")

Method calls in VBA don't require parentheses - these brackets actually force an expression evaluation, that's all they're doing. And in other contexts this forced evaluation can introduce hard-to-find bugs.

Good error messages have proper punctuation, but the problem isn't much with the message, than with how the error is handled:

Resume Next

This takes execution back to the line after the one that blew up. Normally you resume next when you know you've recovered from the error and that everything is stable enough to just resume with the next instruction - it's not the case here. This handler is basically On Error Resume Next with a message box between the error and the resume next. Bad things can happen - if an error occurred and you don't know what caused it, nor whether you can actually recover from it, it's usually best to simply exit the procedure, aborting the process.

Note that in VBA the colon (:) is an instructions separator (except when it's identifying a label), used for stuffing multiple instructions on the same line. Hence, it's completely superfluous here:

On Error GoTo errorHandler:

Because the "next instruction" is a line terminator, a non-instruction.

---

I merely scratched the surface here, there's a lot to say about this code - I'll leave some for other reviewers :)

Answer 2

Error Handling

This immediately jumped out at me.

errorHandler:
MsgBox ("There was an error adding this Change Request")
Resume Next

I'm happy to see that you're attempting to do some error handling, but this...this isn't going to work. You're telling the code to alert the user to an error, but not telling the user what error, and then telling the code to go right back to what it was doing as if everything is hunky-dory and the error magically disappeared. I would rather see no error handling at all than what you've done. At least the code would break on the offending line that way.

Minimally, I would report out the error number and message. Here's a canned function that I use to display error messages in my apps. I also recommend taking a look at these questions for other error handler related advice.

Public Sub messageBox(moduleName As String, procName As String, Optional style As VbMsgBoxStyle = vbCritical, Optional OptionalText As String = vbNullString)
    Dim messageText As String
    messageText = "Module: " & moduleName & vbNewLine & _
                "Procedure: " & procName & vbNewLine & _
                "Source: " & Err.Source & vbNewLine & _
                Err.Description

    If Not OptionalText = vbNullString Then
        messageText = messageText & vbCrLf & OptionalText
    End If

    MsgBox messageText, style, "Runtime Error: " & Err.Number

End Sub

Private Sub exampleCall()
Const PROC_NAME As String = "exampleCall"

    'messageBox "ErrHandler", "exampleCall", vbExclamation
    'ErrHandler.messageBox MODULE_NAME, PROC_NAME
    ErrHandler.messageBox "ErrHandler", "exampleCall", , "Some more info about a particular error"

End Sub

This isn't good either.

        With Application
        .ScreenUpdating = True
        .EnableEvents = True
        End With

Exit Sub

errorHandler:
MsgBox ("There was an error adding this Change Request")
Resume Next

With Application
    .ScreenUpdating = True
    .EnableEvents = True
End With

End Sub

You're duplicating code when you could just re-arrange the execution flow a little bit so that you have the application related code always execute before the subroutine quits. Note that this version stops your code if there's an error.

CleanExit:
    With Application
        .ScreenUpdating = True
        .EnableEvents = True
    End With

Exit Sub

errorHandler:
    MsgBox ("There was an error adding this Change Request")
    Resume CleanExit

End Sub

Single Responsiblility

I have a mantra.

Each subroutine should fit nicely on a single screen.

This kind of mind set has a few advantages. First, it makes it more likely that an entire routine can be understood easily. Secondly, it helps to enforce the Single Responsibility Principle. Each routine should do exactly one thing, and do it well. The single screen thing isn't fool proof though. Your code doesn't scroll much, but it's still doing too much.

At a glance, this code:

1. Gets User Input
2. Finds the Last Row of two different Sheets
3. Evaluates a match formula
4. Copies values from one sheet to another

So, you should potentially have at least 3 distinct functions and a subroutine here. For example, it would be trivial to make a function out of your user input section.

Private Function GetUserConfirmation() as Integer
    GetUserConfirmation = MsgBox("Are you sure you want to update the Accepted Change Requests List?", vbYesNo, "Update Accepted Change Requests")
End Function

Also, please note that MsgBox returns an integer, not a string. (Or more propertly, a VbMsgBoxResult.)

Then you have two options to use this function, depending on how you feel about GoTo.

Option 1

If GetUserConfirmation = vbNo Then
    MsgBox ("No changes saved")
    GoTo CleanExit
End If

Option 2

If GetUserConfirmation = vbYes Then

    ' simply wrap the rest of the code in this if statement.

Else
    MsgBox ("No changes saved")
End If

CleanExit:

Don't forget to extract LastRow and your Evaluate formula out into their own methods as well. It will make the code more readable. (Besides, you'll use that LastRow function everywhere anyway.)

General Notes

• I really like this. I think it's a pretty slick way to see if the destination sheet already has the entry. I tend to forget that Evaluate is there, and this is a good use of it.

  If Evaluate("ISERROR(MATCH(A" & i & ",'Accepted Change Requests'!A:A,0))") Then
  

• You can access a range this way, but consider using Cells as an alternative.

  DestSheet.Range("A" & LastRowDestSheet + 1 & ":D" & LastRowDestSheet + 1).Value
  

  compared to

  With DestSheet
      .Range(.Cells(LastRowDestSheet + 1, 1), .Cells(LastRowDestSheet + 1, 4)).Value
  End With
  

  It may be six of one, or half dozen of the other, but the Cells method forgoes any messy string concatenation. (I consider this to be a matter of preference, just offering an alternative.)