4
\$\begingroup\$

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
\$\endgroup\$
1

2 Answers 2

Reset to default
3
\$\begingroup\$

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

\$\endgroup\$
3
  • 2
    \$\begingroup\$ I think the checkmark is a bit premature - I know a duck that probably has an answer brewing, with more substance - as I said this is just scratching the surface ;) \$\endgroup\$ Commented Jan 14, 2015 at 21:20
  • \$\begingroup\$ I'd feel terrible if I took it away now! \$\endgroup\$ Commented Jan 14, 2015 at 21:22
  • \$\begingroup\$ No problem, I'll take it! But I might also come back later with another review :) \$\endgroup\$ Commented Jan 14, 2015 at 21:23
3
\$\begingroup\$

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

\$\endgroup\$
4
  • 1
    \$\begingroup\$ Resume CleanExit: should be Resume CleanExit ;) \$\endgroup\$ Commented Jan 14, 2015 at 21:25
  • \$\begingroup\$ Old habits die hard. Thanks. I'll fix that. \$\endgroup\$ Commented Jan 14, 2015 at 21:29
  • \$\begingroup\$ Thank you both so much for your help. At the moment I'm struggling, without seeing the fully re-written code alongside your suggestions to fully understand all the of the ideas and principles set out but that might just be because I've been at my desk for 15 hours! \$\endgroup\$ Commented Jan 14, 2015 at 21:53
  • \$\begingroup\$ You're welcome. Come back Anytime. \$\endgroup\$ Commented Jan 14, 2015 at 21:54

You must log in to answer this question.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.