1

Have the following If statement that should only allow the sub to continue if User_ID = 1 or the User_ID = the author ID.

When debugging I know that the first part is true (the user ID is 1), but it still trips the popup.

Stared at this for a while - must have brain fog as it looks like it should work as intended

If (Not User_ID = 1 OrElse Not User_ID = WL_UserID) Then
    AppBoxValidation("Only the original author can edit this item!")
    Exit Sub
End If
David -
  • 2,009
  • 1
  • 13
  • 9
gchq
  • 1,603
  • 2
  • 27
  • 52
  • I agree with Yuriy. I used to get in major trouble on the AND versus OR in If statements Even though logically it was right the If was looking for a different format. – Kat Aug 12 '14 at 20:04

5 Answers5

3

Readability is an important attribute of code:

If (User_ID = 1 Or User_ID = WL_UserID) Then
   'continue
Else 
   AppBoxValidation("Only the original author can edit this item!")
   Exit Sub
End If

Alernatively, you could write:

If (User_ID <> 1 AndAlso User_ID <> WL_UserID) Then
   AppBoxValidation("Only the original author can edit this item!")
   Exit Sub
End If
T McKeown
  • 12,971
  • 1
  • 25
  • 32
  • I was being lazy and just wanted to insert something at the beginning of the code.... Thank you – gchq Aug 12 '14 at 20:18
  • Re the edited version - I tried that first before the NOT prefix and it still threw up the pop-up - that was what threw me out – gchq Aug 12 '14 at 20:28
2

If Not User_ID = 1 is false (because User_ID = 1 is false), then Not User_ID = WL_UserID is true. That's all there is to it .. but, the logic error can be seen as follows.

The initial expression,

Not User_ID = 1 OrElse Not User_ID = WL_UserID

can be rewritten as 

Not (User_ID = 1 AndAlso User_ID = WL_UserID)

by application of De Morgan's Law. This reads as: "It is not the case that User_ID is 1 and User_ID is WL_UserID". Assuming that 1 <> WL_UserID, then the inner AndAlso expression is always false (as User_ID cannot be equal to two different values at once) and the entire expression is true - always.

On way to solve the original, which is what I recommend, is to have one Not on the outside:

Not (User_ID = 1 OrElse User_ID = WL_UserID)

The changed expression reads as: "It is not the case that User_Id is 1 or User_Id is WL_UserID".

However, taking the now correct expression, it is trivial to transform it back to an equivalent with the Not's inside, again by application of DM. (I'm not a fan of this construct and avoid it most of the time.)

Not User_ID = 1 AndAlso Not User_ID = WL_UserID

Or, with transformation of the comparison operators (Not x = y -> x <> y),

User_ID <> 1 AndAlso User_ID <> WL_UserID
Community
  • 1
  • 1
user2864740
  • 60,010
  • 15
  • 145
  • 220
1

As it is if the current User_ID is not 1 or if the User_ID is not equal to WL_USERID then the popup will appear. But from your description it sounds like it should be if the User_ID = 1 or User_ID = WL_UserID then show the pop-up

If (User_ID = 1 OrElse User_ID = WL_UserID) Then
    AppBoxValidation("Only the original author can edit this item!")
    Exit Sub
End If
jormigo
  • 31
  • 6
1

I'd write it this way: "if the user is not the admin, and the user is not the author", disallow the action.

If (User_ID <> 1 AndAlso User_ID <> WL_UserID) Then
    AppBoxValidation("Only the original author can edit this item!")
    Exit Sub
End If

This clearly shows the logic, and is shorter.

Blorgbeard
  • 101,031
  • 48
  • 228
  • 272
0

If the user ID is 1, Not User_ID =1 will evaluate to false, and the code to the right of the OrElse will be evaluated. See this page on the OrElse operator, specifically the Remarks section.

hunch_hunch
  • 2,283
  • 1
  • 21
  • 26