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 = FalseEnd WithOn 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 IfDim SourceRange As Range, DestRange As RangeDim DestSheet As Worksheet, SourceSheet As WorksheetDim 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 WithExit SuberrorHandler:MsgBox (There was an error adding this Change Request)Resume NextWith Application    .ScreenUpdating = True    .EnableEvents = TrueEnd WithEnd Sub
Copying cells from one sheet to another
beginner;vba;excel
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 RangeDim DestSheet As Worksheet, SourceSheet As WorksheetDim 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 LastRowSourceSheetLike this:Dim SourceRange As Range, DestRange As RangeDim DestSheet As Worksheet, SourceSheet As WorksheetDim LastRowDestSheet As Long, i As Long, LastRowSourceSheet As LongSet DestSheet = ThisWorkbook.Worksheets(Accepted Change Requests)Set SourceSheet = ThisWorkbook.Worksheets(All Change Requests)LastRowDestSheet = DestSheet.Cells(DestSheet.Rows.Count, A).End(xlUp).RowLastRowSourceSheet = SourceSheet.Cells(SourceSheet.Rows.Count, E).End(xlUp).RowFor i = 2 To LastRowSourceSheetI 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 NextThis 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 :)