CODEWITHSUNDEEP
vbafor-loopms-word
MistF
MistF

Reputation: 33

My VBA for loop in MS word 2016 is not working

I having trouble with the following code in MS word VBA

For i = 6 To ActiveDocument.Tables(2).Rows.Count
    z = Len(ActiveDocument.Tables(2).Cell(i, 2).Range.Text) - 2
    x = Len(ActiveDocument.Tables(2).Cell(i, 3).Range.Text) - 2
    b = ActiveDocument.Tables(2).Cell(i, 5).Range.ContentControls.Item(1).ShowingPlaceholderText

    If (z = 0 And x = 0) Then
        If b = True Then
            MsgBox "Please do error 1!"
            If vbOK Then
                Exit Sub
            End If
        Else
            MsgBox "Please do error 2!"
            If vbOK Then
                Exit Sub
            End If
        End If
    Else
        If b = True Then
            MsgBox "Please do error 3!"
            If vbOK Then
                Exit Sub
            End If
        Else
            Confirm = MsgBox("Are you sure to submit?", vbYesNo, "Confirmation")
            If Confirm = vbNo Then
                Exit Sub
            End If
        End If
    End If
Next i

The for loop won't go into the second line to check if z or x is having value or not

Upvotes: 0

Views: 213

Answers (1)

AJD
AJD

Reputation: 2438

I doubt moving Next i would have solved anything. This code is riddled with badness.

My impression is that your code is intended to check three columns in a table (from rows 6 downwards) - this appears to be a consistency check.

Naming. z, x and b are not very descriptive. Using names like lengthCol2, lengthCol3 and hasPlaceHolderText will help you follow your logic more closely.

Use Option Explicit. Always.

You use a standard MsgBox call, which by default only has a single button ("OK"). The MsgBox is a blocking code element, so the macro will not progress until the user has clicked "OK".

vbOK is an enumerated value (value = 1). So If vbOK then always comes out true. Always. You appear to be seeking some sort of user input, but you are not clear on what that input is.

Address these simple steps gives us:

For i = 6 To ActiveDocument.Tables(2).Rows.Count
    lengthCol2 = Len(ActiveDocument.Tables(2).Cell(i, 2).Range.Text) - 2
    lengthCol3 = Len(ActiveDocument.Tables(2).Cell(i, 3).Range.Text) - 2
    hasPlaceHolderText = ActiveDocument.Tables(2).Cell(i, 5).Range.ContentControls.Item(1).ShowingPlaceholderText

    If (lengthCol2 = 0 And lengthCol3 = 0) Then
        If hasPlaceHolderText = True Then
            MsgBox "Please do error 1!"
            Exit Sub
        Else
            MsgBox "Please do error 2!"
            Exit Sub
        End If
    Else
        If hasPlaceHolderText = True Then
            MsgBox "Please do error 3!"
            Exit Sub
        Else
            Confirm = MsgBox("Are you sure to submit?", vbYesNo, "Confirmation")
            If Confirm = vbNo Then
                Exit Sub
            End If
        End If
    End If
Next i

Your logic is negative-biased - that is, intending to find reasons not to do something than to do something. Positive-biased logic is usually easier to understand and maintain - the coder's intent is clearer.

Rewording the logic gives us:

For i = 6 To ActiveDocument.Tables(2).Rows.Count
    lengthCol2 = Len(ActiveDocument.Tables(2).Cell(i, 2).Range.Text) - 2
    lengthCol3 = Len(ActiveDocument.Tables(2).Cell(i, 3).Range.Text) - 2
    hasPlaceHolderText = ActiveDocument.Tables(2).Cell(i, 5).Range.ContentControls.Item(1).ShowingPlaceholderText

    If (lengthCol2 > 0 OR lengthCol3 > 0) AND hasPlaceHolderText Then
         Confirm = MsgBox("Are you sure to submit?", vbYesNo, "Confirmation")
         If Confirm = vbYes Then
             'Do submission code here - or call the submission procedure
         End If  ' Just do nothing if they say "No" - this is what your current code does.
    Else
        ' The next line could be used instead of the nested IF-the-else statements following.
        'MsgBox " Table contents are not valid, please ensure columns 2,3 and 5 are completed"
        If hasPlaceHolderText then
            If (lengthCol2 = 0 And lengthCol3 = 0) Then
                MsgBox  "Please do error 1!"
            Else
                MsgBox  "Please do error 2!"
            EndIF
        Else
            MsgBox  "Please do error 3!"
        End If
    End If
Next i

Note that in your logic, either Column 2 or Column 3 can be empty and (as long as placeholder text not being shown) you document is ready for submission. Perhaps you meant AND instead of OR (i.e. all columns should be filled).

There is still one problem. Your loop. As currently written, you loop over the logic, thus you ask the user to either check errors or submit the document x number of times based on error checking in each row. But, just moving the Next i does not solve the problem because the only results that are retained are those in the last row. In other words, all the previous rows could be bad/invalid, but you would still be able to submit.

We can fix this last bit by creating cumulative logic. In other words, we track the errors in a short loop, then go into the main logic. This seems to be a little more complex but it really is relative straight forwards. But, we do need more Booleans to make it work. 

Dim rowsOK as Boolean
'explicit initialisation - I am working on a positive bias here.
    rowsOK = True

For i = 6 To ActiveDocument.Tables(2).Rows.Count
    Dim lengthCol2OK as Boolean  ' Use these just to make the logic clearer and the code cleaner
    Dim lengthCol3OK as Boolean
    Dim hasPlaceHolderTextOK as Boolean
    lengthCol2OK = Len(ActiveDocument.Tables(2).Cell(i, 2).Range.Text) > 2
    lengthCol3OK = Len(ActiveDocument.Tables(2).Cell(i, 3).Range.Text) > 2
    hasPlaceHolderTextOK = ActiveDocument.Tables(2).Cell(i, 5).Range.ContentControls.Item(1).ShowingPlaceholderText
    rowsOK = rowsOK And ((lengthCol2OK Or lengthCol3OK) And hasPlaceHolderTextOK)  ' Note: Using "Or" here as per original code logic
    ' Extra logic could go here to message the user if any of the above are false.
Next i

If rowsOK Then
    Confirm = MsgBox("Are you sure to submit?", vbYesNo, "Confirmation")
    If Confirm = vbYes Then
        'Do submission code here - or call the submission procedure
    End If  ' Just do nothing if they say "No" - this is what your current code does.
Else
    MsgBox " Table contents are not valid, please ensure columns 2,3 and 5 are completed"
End If

However, this logic works on all the rows, so identifying individual row errors in not possible in the main loop. You could work extra logic in the For-Next loop to message the user for errors.

Now the code is maintainable, and more likely does what you want.

Key points:

  • Use Option Explicit. This prevents typos and ensures that you are using variables in they way you intend.
  • Use meaningful variable names. Makes it easier to follow what you want to do.
  • Don't confuse enumerated values with returns from functions. Don't confuse constants with variables.
  • Take some time to review your logic chains to ensure they do what you want to do, rather than not do what you don't want to do. The latter has greater chance of missing a non-valid path.

Upvotes: 1

Related Questions