Copying cells from one sheet to another

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(

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 :)

StackExchange Code Review Q#77537, answer score: 3