2

I would like to get your code thought and views on using conditional vs logical testing.

For example:

To test the conditions of truthness of all of the following variables, their currect status is as follows:

a= True

b= True

c= True

d= True

e= True

One could use as test as follows to ensure that they all are true:

  1. Method 1:

If a And b And c And d And e Then x = "Passed" Else x = "Failed"
  1. Metod 2:
If a = b = c = d = e Then x = "Passed" Else x = "Failed"
  1. Method 3:
If a Then If b Then If c Then If d Then If e Then x = "Passed" Else x = "Failed"

With all variables being "True": Method 2 is the slowest and Method 3 using Condition is fastest.

If any of the variables states would be changed from True to False then Method 3 (Conditional) always wins by big margin.

I do tend to test 2 conditions or more for truthness using the AND logic (we all do, i guess). But isn't quicker to do an "If If" statement.

Looking forward to your valuable feedback and input.

kind regards

Mathieu Guindon
  • 1,720
  • 16
  • 33
Mohsen Alyafei
  • 135
  • 9

4 Answers4

11
If a Then If b Then If c Then If d Then If e Then x = "Passed" Else x = "Failed"

That is some horrendous code right there, and it doesn't do what you think it does either: the 3 "methods" you're showing are NOT equivalent. If we expand #3...

If a Then
    If b Then
        If c Then
            If d Then
                If e Then
                    x = "Passed"
                Else
                    x = "Failed"
                End If
            End If
         End If
     End If
 End If

...we see much more clearly that the only way to get x = "Failed" is to have all but e be True - x remains unassigned in every single other case.

It doesn't matter which method is the fastest, if the fastest method doesn't work as it should.

In fact, it doesn't matter which method is the fastest, if reading that code is deceptive and looks like it should do something, but when you actually stop and read what's going on, you realize it's doing something else.

Write code that's correct first. Then make that code obviously correct. Then make it more efficient if it needs to be made more efficient: the vast majority of the time, if your code has a performance bottleneck, it's not going to be in places like this.

"Method 2" will wrongly output "Passed" if a and b are both False and everything else is True.

"Method 1" is the only code you've got that will consistently output "Passed" when all conditions are true, and "Failed" when one of the conditions is false.

Don't try to write clever code. Write code that works, and then refactor as needed.

What's [slightly] inefficient about #1 is that it evaluates all conditions, even if a is False - in languages such as C# or Java, the condition would "short-circuit" and the rest of the expression wouldn't be evaluated. In VB.NET, the AndAlso short-circuiting operator would do the same.

If profiling code execution reveals that evaluating these conditionals is a performance bottleneck, then the first step is to take #1 and nest the conditions.

x = "Failed"
If a Then
    If b Then
        If c Then
            If d Then
                If e Then
                    x = "Passed"
                End If
            End If
        End If
    End If
 End If

Now we still get correct results, and only evaluate all conditions if we have to. Problem is, that's arrow code, and it's annoyingly ugly.

So what we do is, we write a function whose role is abstract away the iteration a bunch of conditions, and bail out as soon as one of them is False:

Public Function All(ParamArray values() As Variant) As Boolean
    Dim i As Long
    For i = LBound(values) To UBound(values)
        If Not CBool(values(i)) Then Exit Function
    Next
    All = True
End Function

And now we get performant short-circuiting logic* and readability:

x = IIf(All(a, b, c, d, e), "Passed", "Failed")

* Assuming a, b, c, d, and e are all values and not expressions. Expressions would all have to be evaluated in order for their results to be passed as arguments to the function.

Mathieu Guindon
  • 1,720
  • 16
  • 33
  • how will Method 2 work if it is in brackets i.e. if (a=b=c=d=e). Will the condition work? – Mohsen Alyafei Jul 08 '19 at 17:16
  • 1
    Exactly the same. Expressions are evaluated left-to-right, so `a=b` first, then take the result of that, compare to `c`; take the result of that, compare to `d`; take the result of that, compare to `e`. Surrounding the expression in parentheses makes no difference. – Mathieu Guindon Jul 08 '19 at 17:17
  • We should elemenate Metod 2 as unsuitable. Left with Metod 1 or 3 (Conditional or Logical) – Mohsen Alyafei Jul 08 '19 at 17:27
  • There is no distinction between what you call "logical" and "conditional". `If foo` is equivalent to `If foo = True`. – Mathieu Guindon Jul 08 '19 at 17:29
  • I found that method 3 using If on 2 conditions is faster. Do you agree?. e.g. (If A And B) vs ( If A If B) – Mohsen Alyafei Jul 08 '19 at 17:30
  • 1
    This discussion is pointless, as thoroughly explained in this answer... "Method 3" is incorrect and confusing. Don't do that. – Mathieu Guindon Jul 08 '19 at 17:31
  • Let us [continue this discussion in chat](https://chat.stackexchange.com/rooms/95876/discussion-between-mohsen-alyafei-and-mathieu-guindon). – Mohsen Alyafei Jul 08 '19 at 18:12
  • 1
    Write code that expresses your intent first. Correct first is simply fantasy and even if achieved the world will change until correct is wrong. Show me your intent and when I show up later I'll be able to figure out what to do. – candied_orange Jul 08 '19 at 22:57
  • @candied_orange fair enough. Code that correctly expresses intent is likely correct though ;-) – Mathieu Guindon Jul 08 '19 at 23:07
  • Ive been coding long enough that hearing someone say "this code is likely correct" feels like watching someone stare into the infinite universe while they say, "oh I've seen all that before". – candied_orange Jul 08 '19 at 23:15
  • @candied_orange no need to stare into the void of the universe, the point was *first get it to work as intended, THEN worry about whether it's more efficient to write it this way or that way*. – Mathieu Guindon Jul 08 '19 at 23:22
5

I feel I must insist on readable, flexible, debugable code. 

If a = False Then Return "Failed"
If b = False Then Return "Failed"
If c = False Then Return "Failed"
If d = False Then Return "Failed"
Return "Passed"

Or more simply

Debug.Assert a
Debug.Assert b
Debug.Assert c
Debug.Assert d

Please do not couple together separate ideas needlessly. Please do not punish me with needless indentation. Please do not push forms beyond their readable limits simply because they were readable when they had fewer ideas in them.

There is no structure so correct that it won't require restructuring if you push it beyond its readable limits. In fact, improving readability is the best reason to restructure.

candied_orange
  • 102,279
  • 24
  • 197
  • 315
  • You know what, you're absolutely right. KISS wins all the time! (see edit on my answer) – Mathieu Guindon Jul 09 '19 at 01:17
  • I'm working with a library that has lots of assertions like assert (condition1 && condition2 && condition3) instead of having three asserts, so when the assert failed I can then start chasing down which of the three conditions was the reason. While I wouldn't say that it is the most readable code, it is certainly the most debuggable one, which can be very important. – gnasher729 Nov 14 '19 at 17:48
1

You can do in following way also.

if(!A)
  then return "Failed";
else
  if(!B)
    then return "Failed";

This could increase overall performance.

Ishan Shah
  • 339
  • 1
  • 9
0

The speed and readability don't matter, because two of your three different styles give completely wrong results.

a = b = c = d = e most definitely doesn't do what you think it does.

And method 3 leaves x undefined unless a, b, c and d are all true.

gnasher729
  • 42,090
  • 4
  • 59
  • 119