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
