Best Practices - Single Line If Statements?

  • (2 Pages)
  • +
  • 1
  • 2

26 Replies - 1978 Views - Last Post: 08 April 2013 - 10:46 PM

#1 depricated  Icon User is offline

  • RISC Architecture is going to change everything.

Reputation: 929
  • View blog
  • Posts: 3,009
  • Joined: 13-September 08

Best Practices - Single Line If Statements?

Posted 01 April 2013 - 08:43 AM

So at someone's recommendation here I picked up and read Clean Code. I found it incredibly helpful, actually.

Anyway, I wanted opinions. The author of Clean Code had said that single-line if-statements are a no-no. Why?

Now perhaps I could have done this function a bit better, but the idea I went with is that a function should do one thing and do it well - in this case it assigns necessary values to an email template, like so

If Validate(strDescription) Then mailData.setShortDescription strDescription Else Exit Function


There are about 7 of those. I think having 7 lines of code, in this case, is better than the 35 it would take to break that up as

If Validate(strDescription) Then
    mailData.setShortDescription strDescription
Else
    Exit Function
End If


Is there a reason I would want to do 35 lines instead of 7 for this? Does the formatting help?

Is This A Good Question/Topic? 0
  • +

Replies To: Best Practices - Single Line If Statements?

#2 andrewsw  Icon User is online

  • It's just been revoked!
  • member icon

Reputation: 3834
  • View blog
  • Posts: 13,583
  • Joined: 12-December 12

Re: Best Practices - Single Line If Statements?

Posted 01 April 2013 - 08:49 AM

You do not pay per line and your first statement is very difficult to read; I think this is reason enough not to use it.
Was This Post Helpful? 1
  • +
  • -

#3 modi123_1  Icon User is online

  • Suitor #2
  • member icon



Reputation: 9579
  • View blog
  • Posts: 36,292
  • Joined: 12-June 08

Re: Best Practices - Single Line If Statements?

Posted 01 April 2013 - 08:53 AM

First off line count over if statements is silly. Let me get that out of the way before I indulge this.

Quote

Is there a reason I would want to do 35 lines instead of 7 for this? Does the formatting help?

Second, really? Readability, easing understanding, and the like (you know .. all those 'intangibles') make up the bulk of the difference.

If you have been programming for a while you know what an if statement looks like. you know the pattern.. you know that if you see an if condition you can look down the formatting for the associated else or end if.. so you have an idea of what is the entire implication of that portion of the if statement. Having it all one line destroys that ability. That just isn't acceptable in a real world coding environment.

My day is tracked by my ability to generate code so my team (and my future self) can easily track the flow of the application, as well as quickly finding and fixing bugs, and also running updates with out disruption. Throwing me off with some cockamamie non-If pattern just burns my time and annoys me. Also I am guaranteed a lack of comment or just some equally waste of space to explain what the coder did.

It reminds me of this comic:

Posted Image
http://abstrusegoose.com/483
Was This Post Helpful? 1
  • +
  • -

#4 jon.kiparsky  Icon User is offline

  • Pancakes!
  • member icon


Reputation: 8029
  • View blog
  • Posts: 13,741
  • Joined: 19-March 11

Re: Best Practices - Single Line If Statements?

Posted 01 April 2013 - 09:01 AM

Not having read this book, I'm not sure if this is correct in this case but usually "single line if" refers to something like

if (condition)  
  doSomething();


ie, omitting the curly braces. Best practice in algol-style languages would be to include the curly braces:
if (condition) {  
  doSomething();
}



This forestalls a future error where someone tries to add a second statement to the if and fails to add the brackets. Yes, this happens, even to experienced programmers.

Now of your two examples, the latter is a lot easier to read, because the logical structure is mirrored by the layout, so I can read it straight through, correctly.

Generally speaking, minimizing lines of code is not an interesting or useful goal. It's much more important to increase clarity, whether that's done by adding or removing lines. In this case, removing line breaks hurts clarity, so don't do it.

However, minimizing repetition of code is very useful indeed, since duplicated code provides more "bug habitat" - each repeated line is a new chance to get it wrong, especially when you make changes. You say this function is repeated about seven times. I'd look for a way to address that problem instead of trying to mask it by removing newlines.
Was This Post Helpful? 1
  • +
  • -

#5 AdamSpeight2008  Icon User is offline

  • MrCupOfT
  • member icon


Reputation: 2271
  • View blog
  • Posts: 9,500
  • Joined: 29-May 08

Re: Best Practices - Single Line If Statements?

Posted 01 April 2013 - 09:15 AM

I would use the complimented version here.
If Not Validate(strDescription) Then Exit Function
mailData.setShortDescription = strDescription ' <-- Are you missing an assignment here?



The first part is the sanity check.


If you're repeating sections of code, does not that suggest to you make it a function and call that.

This post has been edited by AdamSpeight2008: 01 April 2013 - 10:11 AM

Was This Post Helpful? 1
  • +
  • -

#6 depricated  Icon User is offline

  • RISC Architecture is going to change everything.

Reputation: 929
  • View blog
  • Posts: 3,009
  • Joined: 13-September 08

Re: Best Practices - Single Line If Statements?

Posted 01 April 2013 - 09:31 AM

Thanks guys that's exactly the kind of feedback I was hoping for.

The full body of the function looks something like this:

With mailData
        .setDBValues "Update", "Silver"
        If addrSilver <> "" Then .setBCC addrSilver
        If Validate(strDescription) Then .setShortDescription strDescription Else Exit Function
        If Validate(dateIncidentStart) Then .setStartTime dateIncidentStart Else Exit Function
        If Validate(lngXXX) Then .setXXX lngXXX Else Exit Function
        If Validate(stfYYY) Then .setYYY stfYYY Else Exit Function
        If Validate(strSituation) Then .setSituation strSituation Else Exit Function
        If Validate(strImpact) Then .setImpact strImpact Else Exit Function
        If Validate(strActions) Then .setFix strActions Else Exit Function
        If Validate(dateNextUpdate) Then .setNextUpdate dateNextUpdate Else Exit Function
        If Validate(strPriority) Then .setPriority strPriority Else Exit Function
    End With


or

    With mailData
        .setDBValues "Update", "Silver" 
        
        If addrSilver <> "" Then
            .setBCC addrSilver
        End If
        
        If Validate(strDescription) Then
            .setShortDescription strDescription
        Else
            Exit Function
        End If
        
        If Validate(dateIncidentStart) Then
            .setStartTime dateIncidentStart
        Else
            Exit Function
        End If
        
        If Validate(lngXXX) Then
            .setXXX lngXXX
        Else
            Exit Function
        End If
        
        If Validate(stfYYY) Then
            .setYYY stfYYY
        Else
            Exit Function
        End If
        
        If Validate(strSituation) Then
            .setSituation strSituation
        Else
            Exit Function
        End If
        
        If Validate(strImpact) Then
            .setImpact strImpact
        Else
            Exit Function
        End If
        
        If Validate(strActions) Then
            .setFix strActions
        Else
            Exit Function
        End If
        
        If Validate(dateNextUpdate) Then
            .setNextUpdate dateNextUpdate
        Else
            Exit Function
        End If
        
        If Validate(strPriority) Then
            .setPriority strPriority
        Else
            Exit Function
        End If
        
    End With


In response to the point that I should reduce the number of times I need to run it - what this is doing is validating controls to make sure they have data in them before assigning the data, and then aborting the function when one fails validation. I had considered making an array of controls and iterating through it, but it leaves the problem of still needing to assign them. I'd love opinions on how to address that specific issue as well, but more I just wanted to see what people had to say about which is better and why. I've refactored it out to the second block, and I now understand why it's better to do it that way - which is what I wanted :)

View PostAdamSpeight2008, on 01 April 2013 - 10:15 AM, said:

I would use the complimented verions here.
If Not Validate(strDescription) Then Exit Function
mailData.setShortDescription = strDescription ' <-- Are you missing an assignment here?



The first part is the sanity check.


If you're repeating sections of code, does not that suggest to you make it a function and call that.

Not missing an assignment. This is VB.NET, and .setShortDescription is a public function of mailData's class which accepts a string, rather than being an exposed property.
Was This Post Helpful? 0
  • +
  • -

#7 baavgai  Icon User is offline

  • Dreaming Coder
  • member icon

Reputation: 5937
  • View blog
  • Posts: 12,862
  • Joined: 16-October 07

Re: Best Practices - Single Line If Statements?

Posted 01 April 2013 - 09:35 AM

All those exit functions... The point of not doing stuff in a single line is being able to follow flow. But, you're jumping out all over the place.

How about:
If Validate(dateIncidentStart) Then
	.setStartTime dateIncidentStart
ElseIf Validate(lngXXX) Then
	.setXXX lngXXX
ElseIf Validate(stfYYY) Then
	.setYYY stfYYY
ElseIf Validate(strSituation) Then
	.setSituation strSituation
ElseIf Validate(strImpact) Then
	.setImpact strImpact
ElseIf Validate(strActions) Then
	.setFix strActions
ElseIf Validate(dateNextUpdate) Then
	.setNextUpdate dateNextUpdate
ElseIf Validate(strPriority) Then
	.setPriority strPriority
End If


Was This Post Helpful? 0
  • +
  • -

#8 jon.kiparsky  Icon User is offline

  • Pancakes!
  • member icon


Reputation: 8029
  • View blog
  • Posts: 13,741
  • Joined: 19-March 11

Re: Best Practices - Single Line If Statements?

Posted 01 April 2013 - 09:39 AM

Whichever language this is, it's not one I use, so take this with a grain of salt, but I would suggest that you first validate all of those values, and exit if they fail, and then only set values if they all pass.

OR, if you want, only set the ones that pass. Right now, what you're doing is setting only until one fails, which means you're going to get partial updates if there's a bad value in there, which is probably not what you want. (ie, as you have it now, if you have good values for everything except incident start, then description gets updated, but nothing else does - you probably don't want to update anything in that case, or else to update all values using some "badValue" value for the ones that didn't validate.
Was This Post Helpful? 0
  • +
  • -

#9 depricated  Icon User is offline

  • RISC Architecture is going to change everything.

Reputation: 929
  • View blog
  • Posts: 3,009
  • Joined: 13-September 08

Re: Best Practices - Single Line If Statements?

Posted 01 April 2013 - 09:40 AM

View Postbaavgai, on 01 April 2013 - 10:35 AM, said:

All those exit functions... The point of not doing stuff in a single line is being able to follow flow. But, you're jumping out all over the place.

How about:
If Validate(dateIncidentStart) Then
	.setStartTime dateIncidentStart
ElseIf Validate(lngXXX) Then
	.setXXX lngXXX
ElseIf Validate(stfYYY) Then
	.setYYY stfYYY
ElseIf Validate(strSituation) Then
	.setSituation strSituation
ElseIf Validate(strImpact) Then
	.setImpact strImpact
ElseIf Validate(strActions) Then
	.setFix strActions
ElseIf Validate(dateNextUpdate) Then
	.setNextUpdate dateNextUpdate
ElseIf Validate(strPriority) Then
	.setPriority strPriority
End If



Wouldn't that exit the If statement as soon as the first condition returned true? Which leaves the other elements unvalidated and unassigned.
Was This Post Helpful? 1
  • +
  • -

#10 AdamSpeight2008  Icon User is offline

  • MrCupOfT
  • member icon


Reputation: 2271
  • View blog
  • Posts: 9,500
  • Joined: 29-May 08

Re: Best Practices - Single Line If Statements?

Posted 01 April 2013 - 09:42 AM

I ask because it looks strange. Functions require brackets around their arguments. Eg Test(1) Test(1,2)
The only exception is when the function has no arguments.

Maybe the code highlighter has eaten them.
Was This Post Helpful? 0
  • +
  • -

#11 depricated  Icon User is offline

  • RISC Architecture is going to change everything.

Reputation: 929
  • View blog
  • Posts: 3,009
  • Joined: 13-September 08

Re: Best Practices - Single Line If Statements?

Posted 01 April 2013 - 09:44 AM

View Postjon.kiparsky, on 01 April 2013 - 10:39 AM, said:

Whichever language this is, it's not one I use, so take this with a grain of salt, but I would suggest that you first validate all of those values, and exit if they fail, and then only set values if they all pass.

OR, if you want, only set the ones that pass. Right now, what you're doing is setting only until one fails, which means you're going to get partial updates if there's a bad value in there, which is probably not what you want. (ie, as you have it now, if you have good values for everything except incident start, then description gets updated, but nothing else does - you probably don't want to update anything in that case, or else to update all values using some "badValue" value for the ones that didn't validate.

Hm, could be. . . I'm going to rewrite this and see what I come up with. I'll be back with an update if you wouldn't mind reviewing it?

I'm just trying to learn :) landed my first full time programming gig last week (I've been contracting for 8 months, my first programming contract...) and I want to make sure I'm doing this right. I'm bound to screw stuff up, sadly, but I'm trying.
Was This Post Helpful? 0
  • +
  • -

#12 andrewsw  Icon User is online

  • It's just been revoked!
  • member icon

Reputation: 3834
  • View blog
  • Posts: 13,583
  • Joined: 12-December 12

Re: Best Practices - Single Line If Statements?

Posted 01 April 2013 - 09:45 AM

I don't like the idea of validating, setting a value if an item passes, but abandoning these settings half way through if something fails the validation.

I would, personally, consider creating a List(Of Object) (or perhaps a class), bung all the variables into it, created a boolean (or integer) flag of some kind, then perform the validation in a loop. ONLY if all objects pass the validation would I then set the values.

It seems slightly unusual as well that you would use the same Validate function for strings, dates, etc.. but I suppose this is reasonable.
Was This Post Helpful? 0
  • +
  • -

#13 AdamSpeight2008  Icon User is offline

  • MrCupOfT
  • member icon


Reputation: 2271
  • View blog
  • Posts: 9,500
  • Joined: 29-May 08

Re: Best Practices - Single Line If Statements?

Posted 01 April 2013 - 09:57 AM

It easy do via the help of vb.net array literal syntax { 1,2, 3, 5, 8, 13 }
Plus and little use on of the LINQ extension method. .Any
With mailData
  .setDBValues "Update", "Silver"
  ' Do sanity and validation checks first.
  If addrSilver <> "" Then .setBCC addrSilver
  If {strDescription, dateIncidentStart,      lngXXX, 
              stfYYY,      strSituation,   strImpact,
          strActions,    dateNextUpdate, strPriority }.Any( Function(x) Not Validate(x) ) Then Exit Function
  ' All validation check have passed 
  ' setValues
  .setShortDescription strDescription 
  .setStartTime dateIncidentStart 
  .setXXX lngXXX E
  .setYYY stfYYY 
  .setSituation strSituation 
  .setImpact strImpact 
  .setFix strActions 
  .setNextUpdate dateNextUpdate 
  .setPriority strPriority 
End With



Some time if find it easier to align stuff in the editor, if I turn off vb.net pretty listing feature (via Options -> Text Editor -> VB Specific)
Was This Post Helpful? 0
  • +
  • -

#14 andrewsw  Icon User is online

  • It's just been revoked!
  • member icon

Reputation: 3834
  • View blog
  • Posts: 13,583
  • Joined: 12-December 12

Re: Best Practices - Single Line If Statements?

Posted 01 April 2013 - 10:00 AM

View PostAdamSpeight2008, on 01 April 2013 - 04:42 PM, said:

I ask because it looks strange. Functions require brackets around their arguments. Eg Test(1) Test(1,2)
The only exception is when the function has no arguments.

Maybe the code highlighter has eaten them.

It was valid in VB6 (and VBA) to supply method arguments without using parentheses, but not for function calls. It is no longer valid in VB.NET and if we try to type the arguments without brackets, Visual Studio will insert them.
Was This Post Helpful? 0
  • +
  • -

#15 AdamSpeight2008  Icon User is offline

  • MrCupOfT
  • member icon


Reputation: 2271
  • View blog
  • Posts: 9,500
  • Joined: 29-May 08

Re: Best Practices - Single Line If Statements?

Posted 01 April 2013 - 10:00 AM

Is possible the Validate has overloads, but I've a feeling that they are all Strings.
Was This Post Helpful? 0
  • +
  • -

  • (2 Pages)
  • +
  • 1
  • 2