Solved

Good coding practice - nested if's

Posted on 2001-07-18
40
268 Views
Last Modified: 2008-01-09
I dislike the look of nested if statements and I have been recently using a nested loop.
This also seems to me to be a bit of a bodge so I am wondering what would be the 'correct' way of doing this.

I realise that there is probably no one correct answer so the points will go to the suggestion that I like the best (or that makes most sense).

The procedure reads a line from a file and deals with the content of the line.
Then reads the next line and continues until the end of the file.


nested if:

Private Sub Command1_Click()
 Dim lin As String
 Dim fnum As Integer
    fnum = FreeFile
    Open filename For Input As #fnum
        Do Until EOF(fnum)
            Line Input #fnum, lin
            If condition1 Then
                If condition2 Then
                    If condition3 Then
                        If condition4 Then
                            If condition5 Then
                                DoStuff lin
                            End If
                        End If
                    End If
                End If
            End If
        Loop
    Close (fnum)
End Sub

nested loop:

Private Sub Command2_Click()
 Dim lin As String
 Dim ra() As String
    fnum = FreeFile
    Open filename For Input As #fnum
        Do Until EOF(fnum)
            Do While True
                Line Input #1, lin
                If Not condition1 Then Exit Do
                If Not condition2 Then Exit Do
                If Not condition3 Then Exit Do
                If Not condition4 Then Exit Do
                If Not condition5 Then Exit Do
                DoStuff lin
            Loop
        Loop
    Close (fnum)
End Sub

0
Comment
  • 10
  • 9
  • 8
  • +7
40 Comments
 
LVL 6

Expert Comment

by:JonFish85
ID: 6295435
If condition1 Then
 If condition2 Then
  If condition3 Then
   If condition4 Then
    If condition5 Then
     DoStuff lin
    End If
   End If
  End If
 End If
End If

can become:

If (condition1) and (condition2) and (condition3) and (condition4) and (condition5) Then
  doStuff lin
End if
0
 
LVL 5

Expert Comment

by:KDivad
ID: 6295443
Depends on exactly what Condition1, etc. are. Sometimes, you can do this:

If condition1 And condition2 And condition3 And condition4 And condition5 Then
0
 
LVL 6

Expert Comment

by:JonFish85
ID: 6295445
and this:

If Not condition1 Then Exit Do
If Not condition2 Then Exit Do
If Not condition3 Then Exit Do
If Not condition4 Then Exit Do
If Not condition5 Then Exit Do

can become:

If Not(Condition1) Or Not(Condition2) Or Not(Condition3) Or Not(Condition4) Or Not(Condition5) Then Exit Do
0
 
LVL 5

Expert Comment

by:KDivad
ID: 6295449
<sigh> Too slow again...
0
 
LVL 5

Expert Comment

by:KDivad
ID: 6295460
<<If Not(Condition1) Or Not(Condition2) Or Not(Condition3) Or Not(Condition4) Or Not(Condition5) Then Exit Do>>

Or this:

If Not(condition1 And condition2 And condition3 And condition4 And condition5) Then Exit Do
0
 
LVL 7

Expert Comment

by:Z_Beeblebrox
ID: 6295461
Depending on exactly what you are doing, a select case may work well. Either do a

Select Case variable
   Case 1 ' or whatever
   case 2
End select

or, if your conditions are more complex:

Select Case True
   Case variable > 2 and blnFlag = True
   Case ' etc
End select

Just be aware that if you use the second approach, even if more than one case is true, only the first will be evaluated.

Zaphod.
0
 
LVL 17
ID: 6295482
The conditions are very likely to be function calls which it may be necessary to carry out in order.
eg. if enough data exists an account may be created,if more data exists then an existing account may be updated.

I'm afraid that I find long lines hard to read/debug as well as the nested if.
0
 
LVL 5

Accepted Solution

by:
KDivad earned 50 total points
ID: 6295499
I'm not really suggesting this, but I tend to use something like:

           Line Input #fnum, lin
           If Evaluate(line) Then DoStuff lin

Then put all the nasty stuff in a function called Evaluate or whatever's relevant. It doesn't actually get rid of the problem, but tucks it out of sight...
0
 
LVL 6

Expert Comment

by:VK
ID: 6295529
Private Sub Command3_Click()
    Dim lin As String
    Dim ra() As String
   
    fnum = FreeFile
    Open FileName For Input As #fnum
   
    Do While Not lin.EOF
        Line Input #1, lin
        Select Case True
            Case condition1, condition2, condition3, condition4, condition5
                Exit Do
            Case Else
                DoStuff lin
        End Select
    Loop
   
    Close (fnum)
End Sub
0
 
LVL 28

Expert Comment

by:AzraSound
ID: 6295549
>>I'm afraid that I find long lines hard to read/debug as well as the nested if

you can put the conditions on separate lines:

If (condition1 _
    And condition2 _
    And condition3 _
    And condition4 _
    And condition 5) Then
0
 
LVL 5

Expert Comment

by:KDivad
ID: 6295555
Good point, AzraSound! I always forget about that, since I personally hate it...
0
 
LVL 22

Expert Comment

by:rspahitz
ID: 6295694
Sometimes it's worthwhile to relegate such ugly things to the back crevices of another routine as seen below.  Obviously the conditions may need to be changed depending on the expression's variables.

Private Sub Command1_Click()
Dim lin As String
Dim fnum As Integer
   fnum = FreeFile
   Open filename For Input As #fnum
       Do Until EOF(fnum)
           Line Input #fnum, lin
           TryToDoStuff
       Loop
   Close (fnum)
End Sub


' Shove this off in a corner of the module
Private Sub TryToDoStuff(lin as String)
  If condition1 Then
    If condition2 Then
      If condition3 Then
        If condition4 Then
          If condition5 Then
            DoStuff lin
          End If
        End If
      End If
    End If
  End If
End Sub
0
 
LVL 1

Expert Comment

by:wozza091599
ID: 6295726
i don't think putting all teh condition checks into one long statement is very efficient,  like above...
If (condition1 _
   And condition2 _
   And condition3 _
   And condition4 _
   And condition 5) Then

as teh function has to evaluate everyone before it can make a decision where as with the nestled if statements the code will run faster cos if the first condition isn't met tehn it exits straight away and doesn't go on checking at the other conditions as above... i remember reading a long article by some guru about writing l33t fast code and this was his explanation which i fully agree with and i've tested it... i think it was from the 101 tips for vb from the vb programmers journal 2000...
so i think that your first one would be the fastest.. what type of conditions are you checking perhaps we coudl clean those up???



0
 
LVL 1

Expert Comment

by:wozza091599
ID: 6295730
i don't think putting all teh condition checks into one long statement is very efficient,  like above...
If (condition1 _
   And condition2 _
   And condition3 _
   And condition4 _
   And condition 5) Then

as teh function has to evaluate everyone before it can make a decision where as with the nestled if statements the code will run faster cos if the first condition isn't met tehn it exits straight away and doesn't go on checking at the other conditions as above... i remember reading a long article by some guru about writing l33t fast code and this was his explanation which i fully agree with and i've tested it... i think it was from the 101 tips for vb from the vb programmers journal 2000...
so i think that your first one would be the fastest.. what type of conditions are you checking perhaps we coudl clean those up???



0
 
LVL 22

Expert Comment

by:rspahitz
ID: 6295773
"the function has to evaluate everyone before it can make a decision"

This is true in the current versions of VB.  The VB.Net documentation indicates that this will be fixed for the .net version.
0
 
LVL 15

Expert Comment

by:ameba
ID: 6296503
my $0.02  :-)

- This piece doesn't need "End If"
- Lines are not long
- Code is efficient (if condition1 fails, other conditions are not evaluated)
- There is no need for fake Do...Loop

           If condition1 Then _
              If condition2 Then _
              If condition3 Then _
              If condition4 Then _
              If condition5 Then _
              DoStuff lin: MsgBox "Hello"
0
 
LVL 1

Expert Comment

by:Vinda
ID: 6296957
Select Case False
    Case Condition1, Condition2, condition3, Condition4, Condition5
        'Do nothing
    Case Else
        'do ur stuff here
        DoStuff lin
End Select


0
 
LVL 5

Expert Comment

by:KDivad
ID: 6296998
ameba, Nice! Very nice!
0
 
LVL 17
ID: 6297736
ameba, I think that you've hit the nail right on the head about the fake do loop.
In an old version of some Basic that I once used you were able to re-enter a For loop from any part of the block beneath it.

For c = 1 to 100

if not condition1 then next c
if not condition2 then next c
if not condition3 then next c
if not condition4 then next c

Next c

There is probably something very wrong with this as it is not possible in VB, but I think it's something like this that I am looking for.

Reading the code, if the conditions/functions are named carefully then it becomes very clear what state the program is in at each line (perhaps I should say 'statement').
0
 
LVL 15

Expert Comment

by:ameba
ID: 6297876
RobinD,
>There is probably something very wrong with this

Yes. But after few VB years you get used to it.

"One statement VB lacks is the Continue statement. There are times in the middle of a loop when you want to stop testing and just go to the next iteration in the loop. One can approximate this with nested Ifs but the logic of the nesting can quickly become quite complicated and obscure to follow."

Note that VB has nice function Switch, but it is not very efficient, since all conditions are evaluated.  A workaround to NOT execute all conditions, is to use a flag.  Here is the sample, but, now, those nested IFs don't look so bad.  :-)

Option Explicit
Dim x As Long
Dim blnDone As Boolean

Private Sub Form_Load()
    x = 5
    If Switch(Condition1, 1 _
        , Condition2, 2 _
        , Condition3, 3 _
        , Condition4, 4 _
        , Condition5, 5 _
        ) = 5 Then
       
        MsgBox "Hello"
    End If
End Sub

Private Function Condition1() As Boolean
    If blnDone Then Exit Function ' do not execute the 'meat' of the function
    If x > 0 Then Condition1 = True
    blnDone = True
End Function

Private Function Condition2() As Boolean
    If blnDone Then Exit Function ' do not execute the 'meat' of the function
    If x > 10 Then Condition2 = True
    blnDone = True
End Function

Private Function Condition3() As Boolean
    If blnDone Then Exit Function
    If x > 20 Then Condition3 = True
    blnDone = True
End Function

Private Function Condition4() As Boolean
    If blnDone Then Exit Function
    If x > 30 Then Condition4 = True
    blnDone = True
End Function

Private Function Condition5() As Boolean
    If blnDone Then Exit Function
    If x > 40 Then Condition5 = True
    blnDone = True
End Function


VB has also nice statements "Select Case" and "Choose", but they cannot be used here.
0
Highfive + Dolby Voice = No More Audio Complaints!

Poor audio quality is one of the top reasons people don’t use video conferencing. Get the crispest, clearest audio powered by Dolby Voice in every meeting. Highfive and Dolby Voice deliver the best video conferencing and audio experience for every meeting and every room.

 
LVL 15

Expert Comment

by:ameba
ID: 6297893
OOPS, sorry, Switch cannot be used here.  To strong sun here?  :-)
0
 
LVL 15

Expert Comment

by:ameba
ID: 6298095
>          If condition1 Then _
>             If condition2 Then _
>             If condition3 Then _
>             If condition4 Then _
>             If condition5 Then _
>             DoStuff lin: MsgBox "Hello"

Although this piece of code satisfies all the conditions, you should not use it:
- it is a bit strange to use multiple "If...Then" in a single line (I am surprised that syntax works) and it's hard to read/understand.
0
 
LVL 17
ID: 6298143
I do feel like I'm changing the rules as I go along, I hope it doesn't look like it.

I quite liked the look of:
          If condition1 Then _
             If condition2 Then _
             If condition3 Then _
             If condition4 Then _
             If condition5 Then _
             DoStuff lin: MsgBox "Hello"

although I don't really like line continuation, it does exit nicely and without executing everything.
but I can't add remarks within the block.

If condition1 Then _'check name and address matches
If condition2 Then _'send emails to all your friends
DoStuff lin: MsgBox "Hello"

I do try to name my functions carefully but sometimes a little extra explanation can help.

There is probably no right answer, but please keep on trying.

It's not the executing of the conditions that I am trying to find an alternative for but the removal of my false do_loop.

Isn't there a limit to the depth of nested loops, I know I only put 5 conditions in there but I am looking for a general case.

I did think there may be a text-book answer as quite often you see statements like 'although this works it is bad programming practice', usually with no explanation as to why. Maybe there is a book of 'good programming practice' but I have yet to find it.
0
 
LVL 15

Expert Comment

by:ameba
ID: 6298195
>       Do Until EOF(fnum)
>           Do While True
>               Line Input #1, lin
>               If Not condition1 Then Exit Do
>               If Not condition2 Then Exit Do
>               If Not condition3 Then Exit Do
>               If Not condition4 Then Exit Do
>               If Not condition5 Then Exit Do
>               DoStuff lin
>           Loop
>       Loop

>removal of my false do_loop.
Without Do Loop (note that most programmers don't like GOTOs):

       Do Until EOF(fnum)
           Line Input #1, lin
           If Not Condition1 Then GoTo continue
           If Not Condition2 Then GoTo continue
           If Not Condition3 Then GoTo continue
           If Not Condition4 Then GoTo continue
           If Not Condition5 Then GoTo continue
           DoStuff lin
continue:
       Loop
0
 
LVL 17
ID: 6298515
aaaarrgghh!!!
0
 
LVL 17
ID: 6298603
re: aaarrggh

Sorry, you caught me by surprise there.

I suppose I am trying to GoTo without actually using the word.

The procedure's I write that use this type of nested statements are usually robotics that run through a sequence of actions usually performed by a user, the robotic just makes it easier and a lot faster for them.

Maybe I'm trying to hide the fact that it's sequential.
0
 
LVL 15

Expert Comment

by:ameba
ID: 6298752
>Private Sub Command2_Click()
>Dim lin As String
>Dim ra() As String
>   fnum = FreeFile
>   Open filename For Input As #fnum
>       Do Until EOF(fnum)
>           Do While True
>               Line Input #1, lin
>               If Not condition1 Then Exit Do
>               If Not condition2 Then Exit Do
>               If Not condition3 Then Exit Do
>               If Not condition4 Then Exit Do
>               If Not condition5 Then Exit Do
>               DoStuff lin
>           Loop
>       Loop
>   Close (fnum)
>End Sub

I think this is a bit better:

Private Sub Command2_Click()
Dim lin As String
Dim ra() As String
   fnum = FreeFile
   Open filename For Input As #fnum
       Do Until EOF(fnum)
           Line Input #1, lin
           Do
               If Not condition1 Then Exit Do
               If Not condition2 Then Exit Do
               If Not condition3 Then Exit Do
               If Not condition4 Then Exit Do
               If Not condition5 Then Exit Do
               DoStuff lin
               Exit Do
           Loop
       Loop
   Close (fnum)
End Sub
0
 
LVL 15

Expert Comment

by:ameba
ID: 6298795
or something like this (uses For...Next Loop instead of Do...Loop):

    Line Input #1, lin

    For i = 1 To 5
        Select Case i
        Case 1
            If Not Condition1 Then Exit For
        Case 2
            If Not Condition2 Then Exit For
        Case 3
            If Not Condition3 Then Exit For
        Case 4
            If Not Condition4 Then Exit For
        Case 5
            If Not Condition5 Then Exit For
            DoStuff lin
        End Select
    Next
0
 
LVL 17
ID: 6299089
ameba,
re: >I think this is a bit better:

My mistake, I actually do a function call for the outer loop along the lines of:

Do while myObject.readfile(lin)

myObject deals with opening/closing the file, what type of file it is, rem lines etc.
(hiding the nasty stuff)

I just threw the example together without noticing where line input should have gone.

well spotted.






I'm going to have to award some points for this soon and so far I've seen two offers that interest me.


KDivad's >I'm not really suggesting this, but
(hide all the nasty bit's)

but why are you not suggesting it ?



and

ameba's >Although this piece of code satisfies all the conditions, you should not use it:

if _
if _
if _

why shouldn't I use it?




0
 
LVL 5

Expert Comment

by:KDivad
ID: 6299122
<<but why are you not suggesting it ?>>

Because your question seemed to indicate that you wanted to remove that bit of code, not just hide it. And now that you say MyObject hides all the nasty reading stuff, I would have thought you'd have already thought of this.
0
 
LVL 17
ID: 6299237
KDivad,
Where there is no 'right' answer and I just decide which answer I like best, a suggestion that goes along the lines of the way I tend to code looks like a good one.

MyObject hides some nasty stuff, but you want to see the multitude of sins that lie beneath conditions 1 to n.

Thanks for the reply I just wondered if this was a very bad way of doing things - at least I'm not the only one :7).

 
0
 
LVL 22

Expert Comment

by:rspahitz
ID: 6299252
One thing that should be considered in any "tricky" coding is future readability and maintainability.

Although the single-line multi-IF split at line-breaks looks good, it would initially confuse most programmers.  Plus, since you can't add comments, that further enhinders the ability to clear up what you're doing.

The nested IF blocks get complicated, but are at least commonly accepted as an extension of what we already do with simple nested IF blocks.

I re-iterate that if the IF block is that complicated, it may be worthwhile breaking off a chunk and dropping it into a subroutine or maybe even a function.


Personally, I like Kdivad's solution, but would likely split this into multiple lines (even though I can't add comments):

If condition1 _
And condition2 _
And condition3 _
And condition4 _
And condition5 Then
  DoThis
End If

I suppose it's essentially the same as using the If-Then-If-Then except that it includes the endif that gives me a good feeling (confirmation) that I've actually come to the end of this mess.
0
 
LVL 15

Expert Comment

by:ameba
ID: 6299328
RobinD,
>why shouldn't I use it?

rspahitz wrote >Although the single-line multi-IF split at line-breaks looks good, it would initially confuse most programmers.

Yes, this is the reason.  Most projects are reviewed or programmed by more programmers.
And reading is done many, many times (10x) more then writing.  It is good idea to invest in readability.
0
 
LVL 17
ID: 6299362
Thanks ameba,

I quite like the look of your:

For i = 1 To 5
       Select Case i
       ...

although I think it would be a bit of a fright to step through, I'm sure it will come in useful one day. I've never thought of directly generating the case condition like this.

I've decided to award the points to KDivad as it was an early answer and neatly replaces the false loop with a function call.

ameba - thanks for all your suggestions, look out for a 'points for' as soon as I get one in there.
0
 
LVL 5

Expert Comment

by:KDivad
ID: 6299599
<surprised>
Thanks! Glad we could help.

I also like the look of ameba's For...Next idea. Not sure I'd ever use it, but it does look ingenius.
0
 
LVL 6

Expert Comment

by:VK
ID: 6301195
I have the best solution:

Call DoJob

:-)
0
 
LVL 5

Expert Comment

by:KDivad
ID: 6303225
Short. To the point.

<grin>
0
 
LVL 22

Expert Comment

by:rspahitz
ID: 6303376
Yup, but ultimately it comes back to the accepted solution: hide it in a corner somewhere.
0
 
LVL 5

Expert Comment

by:KDivad
ID: 6303403
Sure does. Just depends on how much of the code Robin is willing to (and can) hide.
0
 
LVL 17
ID: 6307445
The primary purpose of this was to create some readable, easy to step-through code in a 'good coding-practice' way.
The nearest I had got to this was by using my fake do-loop.
It works quite well, you can step through the different procedures and each one does it's own little bit in it's own little way.
The only sore thumb was the do-loop.

KDivad's suggestion placed the contents of the loop in a function - same thing happens, except that you get a perfectly valid 'Exit Function' in place of exit for.

Actually I hadn't thought of this as the procedures that are called are so high level that shoving them down a step just hadn't crossed my mind.

It's not so much about hiding code (It all has to be there for something or I wouldn't have put it in.) as keeping code that does things with other code that does similar things and giving it all access to common procedures where necessary.

I just wondered if there was a 'right' way to do it. If there is this may be it.
0

Featured Post

Maximize Your Threat Intelligence Reporting

Reporting is one of the most important and least talked about aspects of a world-class threat intelligence program. Here’s how to do it right.

Join & Write a Comment

Have you ever wanted to restrict the users input in a textbox to numbers, and while doing that make sure that they can't 'cheat' by pasting in non-numeric text? Of course you can do that with code you write yourself but it's tedious and error-prone …
Enums (shorthand for ‘enumerations’) are not often used by programmers but they can be quite valuable when they are.  What are they? An Enum is just a type of variable like a string or an Integer, but in this case one that you create that contains…
Get people started with the process of using Access VBA to control Outlook using automation, Microsoft Access can control other applications. An example is the ability to programmatically talk to Microsoft Outlook. Using automation, an Access applic…
Get people started with the utilization of class modules. Class modules can be a powerful tool in Microsoft Access. They allow you to create self-contained objects that encapsulate functionality. They can easily hide the complexity of a process from…

743 members asked questions and received personalized solutions in the past 7 days.

Join the community of 500,000 technology professionals and ask your questions.

Join & Ask a Question

Need Help in Real-Time?

Connect with top rated Experts

13 Experts available now in Live!

Get 1:1 Help Now