2
\$\begingroup\$

I've created a macro that finds the letter "O" in a range, then moves it up one cell if the destination cell does not have a "." or an adjacent border.

It works fine, but I've heard that the use of the "GoTo" statement is something that should be avoided as much as possible (leads to spaghetti code, etc.)

Is it okay to use in the following context? I've had a go at tweaking it to not use "GoTo" but haven't had any luck - I'm sure it's a very simple fix but my limited knowledge of VBA is holding me back. I tried using "If Not" instead of "If" but couldn't seem to get it to take into account both the border and the "." (it still moved the "O" into a cell containing a ".")

Sub MoveUp()

    Dim O As Range
    Set O = Range("A1:Z26").Find(What:="O", LookAt:=xlWhole, MatchCase:=True)

        If O.Offset(-1, 0) = "." _
        Or O.Offset(-1, 0).Borders(xlEdgeBottom).LineStyle <> xlNone _
        Then
        GoTo CancelSub
        Else

            O.Offset(-1, 0) = "O"
            O.ClearContents

        End If

CancelSub:

    Set O = Nothing

End Sub

PS. Please excuse me if there's some unorthodox formatting, still getting used to it!

\$\endgroup\$
0
\$\begingroup\$

Your main concern is the GoTo and rightly so. But the main problems caused by the statement become a lot more obvious with complex code, not in your short Sub

The main issue with GoTo is that it creates convoluted logic: If condition go to this label, when you get to this label there might be another condition that will redirect to another place which redirects to yet another place, yet another place back to this label where another condition leads to a new location, and so on until you become completely lost and are barely able to keep up with the last two or three locations, and this creates a huge potential for bugs

The solution to avoiding these jumps is not necessarily simpler, because you will probably need more steps to connect the flow and overcome the disjointed shortcuts created by the GoTo’s. But these extra steps will help later on when maintaining the code because you’ll always be aware of the context: where the logic is coming from and where it leads, causes and effects, and it makes it a lot easier to spot the issues

Back to your Sub, you are aware the formatting is not ideal but you did try to highlight the main “action” by the use of vertical white space – it makes it quite easy to locate. But there are more issues as well:

Option Explicit

  • Missing the first line of defense - any spelling mistake will create a new variable, and the variables you intended to use are not behaving as expected. The compiler will never alert you, and finding these simple issues can be very time consuming in large procedures

Sub MoveUp()

  • The title is too generic; it doesn’t provide clues to the general intent of the Sub which is very specific to the letter O and the cell above it, under certain conditions which are also quite specific. In a larger application you might end up with a dozen “MoveUp” procedures that will probably need to be distinguished by non-intuitive numbers MoveUp1, MoveUp2, MoveUp3, etc. You could name it something like MoveUpAllOs()
  • All subs and functions default to Public, but you should make this intention clear by declaring the Sub as Public (accessible outside of this module)

When I first glanced at the code I inferred that you wanted to move all “O”s accordingly, but the code moves only the first O found in the range. Is this random or you actually wanted to move all of them? Maybe you have a specific reason to search for the first O found “ByRows”, which is the default SearchOrder for the “Find” method. If you want to check all of them you need to use a loop.

Dim O As Range

  • The name does indicate the main subject but it doesn’t convey its purpose. And what happens if the latter changes – will you update all variable names from “O” to “X”? Again, this will be more difficult in larger procedures

Set O = Range("A1:Z26")

  • Unqualified Range reference - what Worksheet are you referring to? Which Workbook? This is the main cause for issue in VBA. The macro recorder creates a bad habit because it implicitly refers to the ActiveWorkbook.ActiveSheet.
    • Be explicit about the full path of the range: ThisWorkbook is where the VBA code resides and executes. Worksheets(“Sheet1”) is identifying the worksheet by the name displayed in the tab, or Worksheets(1) is identifying it by its index as it appears in the tab order, Sheet1 is identifying it using the CodeName property of the Worksheet object which is visible and editable only from the VBA editor window and makes it safer to use – sheet names and indexes are easily changed by the end-user
  • "A1:Z26" is too specific – do you have thousands of rows and columns, and you only want A to Z, row 1 to 26, or is this intended for the entire used range? If it’s all data, you could make it dynamic without the need to update it for every new row or column: Sheet1.UsedRange would be a bit more flexible, yet restrictive enough for the Find to avoid searching 1 million+ rows

If O.Offset(-1, 0) = "."

  • This is expecting the previous search to return a cell, but doesn’t account for the possibility of not finding an O. If the Find returns nothing the “O” range will be Nothing, and O.Offset(-1, 0) will generate "Run-time error '91' (Object variable or With block variable not set)"
  • It’s also assuming that there will always be a cell above the found O. If it’s found in row 1 this will generate "Run-time error '1004' (Application-defined or object-defined error)"

.

The GoTo can be avoided with this If statement, using “And” to combine the conditions:

If O.Offset(-1, 0).Value2 <> "." And _
   O.Offset(-1, 0).Borders(xlEdgeBottom).LineStyle = xlNone Then
        O.Offset(-1, 0) = "O"
        O.ClearContents
End If

Your If translates to a double negative, and it jumps over the main steps:

  • Don't move O if the above is a dot, or it doesn't have a border

This If translates to:

  • Move O if the above is not a dot and it doesn't have a border

.


.

This is how I would re-write the Sub

Move all instances

Option Explicit

Public Sub MoveUpAllOs()
    Const COND_VAL = "."
    Const COND_BRD_LOC = xlEdgeBottom
    Const COND_BRD_TYPE = xlNone
    Dim usedArea As Range, foundItem As Range

    Set usedArea = Sheet1.UsedRange
    Set foundItem = usedArea.Find(What:="O", LookAt:=xlWhole, MatchCase:=True)
    If foundItem Is Nothing Then Exit Sub

    Dim firstFound As String
    firstFound = foundItem.Address
    Dim upperIsFree As Boolean          'declaration to closest usage, outside loop
    Dim started As Boolean
    Application.ScreenUpdating = False
    Do
        If foundItem.Row > 1 Then
            With foundItem.Offset(-1)
                upperIsFree = .Borders(COND_BRD_LOC).LineStyle = COND_BRD_TYPE
                If .Value2 <> COND_VAL And upperIsFree Then
                    .Value2 = foundItem.Value2
                    foundItem.Value2 = vbNullString
                    If Not started Then 'updates first found address, after the move
                        firstFound = .Address
                        started = True
                    End If
                End If
            End With
        End If
        Set foundItem = usedArea.FindNext(foundItem)
    Loop While foundItem.Address <> firstFound
    Application.ScreenUpdating = True
End Sub

Move the first found

Public Sub MoveUpFirstO()
    Const COND_VAL = "."
    Const COND_BRD_LOC = xlEdgeBottom
    Const COND_BRD_TYPE = xlNone

    Dim usedArea As Range
    Dim foundItem As Range

    Set usedArea = Sheet1.UsedRange
    Set foundItem = usedArea.Find(What:="O", LookAt:=xlWhole, MatchCase:=True)

    If foundItem Is Nothing Then Exit Sub

    If foundItem.Row > 1 Then
        With foundItem.Offset(-1)

            Dim upperIsFree As Boolean
            upperIsFree = .Borders(COND_BRD_LOC).LineStyle = COND_BRD_TYPE
            If .Value2 <> COND_VAL And upperIsFree Then
                .Value2 = foundItem.Value2
                foundItem.Value2 = vbNullString
            End If

        End With
    End If
End Sub
\$\endgroup\$
  • \$\begingroup\$ This is such an informative answer - I can't thank you enough. This makes it much easier to make edits to the code and, as you rightly said, greatly decreases the chances of bugs and errors later on. Thank you. In my case, the area in which the "O" will be will always be "A1:Z26", and there should only ever be one "O" in that area. I could change the usedArea to be Sheet1.Range("A1:Z26") but your suggestion works perfectly anyway. I've opted for the "first found" version of your code. \$\endgroup\$ – Antony Jul 11 '17 at 16:40

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.