# Moving a value into another cell IF the destination does not have an adjacent border OR contain certain text

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!

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
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
started = True
End If
End If
End With
End If
Set foundItem = usedArea.FindNext(foundItem)
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

• 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. – Antony Jul 11 '17 at 16:40