Welcome Guest, Not a member yet? Register   Sign In
Avoiding nesting everything inside IF statements
#1

[eluser]BrianDHall[/eluser]
This just occurred to me. Here's something I find annoying that I want to avoid:

Code:
function control_panel()
{
if ($logged_in)
{
// all your code goes here
}
else
{
// they shouldn't be seing this page, so redirect or show login form
}
}

I hate this because it creates all that extra indenting and nesting when it seems so...inelegant. I had seen suggestions about using auth libraries to handle this or control it at a parent-class or constructor level, but this bit of code came to me:

Code:
function preview_listing()
    {
        
        if (!$logged in)
        {
            redirect('login_form');
            return;
        }

                // continue with your coding
        
    }

This should work shouldn't it? Am I missing something?

By "should work", it does infact appear to work, but I want to make sure I'm not breaking something silently that I won't notice until I want to slam my head into the wall.

I'd never seen it done this way before, and just wondered...well, why not?
#2

[eluser]kurucu[/eluser]
No, I use return like that to escape from the controller after displaying errors (e.g. when a specific index is not found, and showing a default is not valid). It hasn't bitten me yet.

I guess you'd have different things in place of $logged_in, if you want to test for a specific role etc?

However... While having code nested inside a bunch of IF statements doesn't look nice, that doesn't mean it is necessarily inelegant code! It might be perfect logic and very efficient, but the human programmer is allergic to it Smile

If have a bunch of
Code:
if( $this->user->can('delete_blog_entries') )
{
    //delete entry/show the form
} else {
    //invite them to log in
    //'inelegant', but will later allow me to (for example) cache the demand
    //so that I can act on it once they do log in
}

If the controller needn't offer complexity, I do exactly what you do
Code:
if(!$page = $this->pagemodel->get_page($id) ){ $this->standard_message('invalid_page'); return; }
// same for permissions
// ... other things to check, all returning with a message if failed

// then show act/show form/process form

So I'm potentially waiting for the same wall to creep up on me!
#3

[eluser]jedd[/eluser]
Brian,

I use that approach all the time, though on occasion (usually when it's something a tad more complex than simply 'if something, go somewhere else') I'll stick with the long block indentation approach.

A few thoughts on the subject.

Long block indentation - these tend to appear uglier once they span more than a screen length. If you have functions that span more than a screen length, some might say that your function is too long, and should be broken down into component parts. Rule of thumb, stuff, to be sure - but something to consider if it's part of the reason you find the big-indented-block unwieldy.

I think you probably don't like it because the negated if conditional seems .. around the wrong way. Don't fret it - it's just a conditional. You're not comfortable with them, because you're not used to seeing them, in turn because you're not used to coding them. The obvious solution - code more of them. Wink

I've read that you should have only one exit point for a given function. I guess they mean grouping of exit points, since you want to return different things based on different inputs. And I can see the argument there, though it's probably something that is easier as you get more proficient (I have exit points scattered everywhere, so I've a long way to go, mind). I think the two approaches you describe, though, have the same number of exit points, and comparable (just inverted) logic that wraps them up, with the only major distinction being that you don't need the 'else ... ' in front of your big block of stuff, because 'else' is implied by the redirect() on the line above.

From a performance point of view - theoretical, at least, because it's unlikely to ever intrude measurably on your actual system performance - you should arguably conduct your fastest tests first in any function. As I say, this is unlikely to ever impact in any significant way, but it does suggest that doing the negated test, and getting the heck out of that function sooner rather than later, scores higher points on the 'better coding approach' test.

(On that last - yes, also, equally arguably, you should do your tests in descending order of number of cases that get culled - but that implies some decent test data, and consequently implies optimisation stage, rather than initial design and coding choices. I'm sticking to that line for the time. Wink
#4

[eluser]BrianDHall[/eluser]
[quote author="kurucu" date="1254254810"]No, I use return like that to escape from the controller after displaying errors (e.g. when a specific index is not found, and showing a default is not valid). It hasn't bitten me yet.

I guess you'd have different things in place of $logged_in, if you want to test for a specific role etc?[/quote]

Actually I just stuck that variable there for simplicity, as what I actually am doing in that particular function is that if a person isn't presently working on an incomplete listing then they have no business trying to 'preview' it (which I determine by checking the existence of an ORM-powered class variable).

But yeah, for user login roles I was thinking of exactly that - sometimes it doesn't matter so long as they are logged in, and sometimes it does Smile

Quote:However... While having code nested inside a bunch of IF statements doesn't look nice, that doesn't mean it is necessarily inelegant code! It might be perfect logic and very efficient, but the human programmer is allergic to it Smile

Its really all the human element, you hit the nail on the head there. The problems I find with it are A) lots of nesting isn't pretty and distracts the eye and gives one more thing to have to fix occasionally if your editor gets confused, B) increased chance of mis-matching brackets, putting code in the wrong bracket, or worst of all putting code that assumes a certain security check in the wrong set of brackets, and C) being inside a loop increases human memory usage and requires concentration (you have to constantly remember you are in a loop and what conditions triggered your arrival there) even if it requires no computer over-head in terms of memory or CPU strain.

As ADDey (thats now a word, I just 'precedented' it) as I tend to be, I can't afford any extra mental overhead Wink

Quote:So I'm potentially waiting for the same wall to creep up on me!

Thanks kurucu, I'm really glad to hear it works as I thought it would as it seems like such a nice option to have in some functions and controllers. Gives me new appreciation for the whole point of pages being 'functions' to begin with - easy to end execution of them without breaking the page or other functions (like exit might do).
#5

[eluser]kurucu[/eluser]
[quote author="jedd" date="1254256006"]I've read that you should have only one exit point for a given function. I guess they mean grouping of exit points, since you want to return different things based on different inputs. And I can see the argument there, though it's probably something that is easier as you get more proficient (I have exit points scattered everywhere, so I've a long way to go, mind). I think the two approaches you describe, though, have the same number of exit points, and comparable (just inverted) logic that wraps them up...[/quote]
I imagine this depends on the programming language, as many require (especially C derivatives) require the return type to be declared. But for all languages, having a single return point means you know where the code exits the function, you can trace every case, and won't end up with lost cases (particularly within nested IF conditionals). Certainly, you could have no implied 'else' cases with a single return point.
But, everyone who has commented so far has given examples where their function is traceably exiting if the conditional is true, or otherwise continuing - no ambiguity or lost cases.

[quote author="BrianDHall" date="1254256494"]Actually I just stuck that variable there for simplicity, as what I actually am doing in that particular function is that if a person isn't presently working on an incomplete listing then they have no business trying to 'preview' it (which I determine by checking the existence of an ORM-powered class variable).

But yeah, for user login roles I was thinking of exactly that - sometimes it doesn't matter so long as they are logged in, and sometimes it does Smile[/quote]Ah yes, I noted that it was probably an example as I hit 'post' Smile Still similar approaches though. I'm not yet taken by the ORM thing, but it might come to me. Something on a similar subject is having long conditionals in the original IF statement. I wrote, but couldn't stand, conditionals over several lines checking to see if a user was an administrator, or could edit all posts in a forum, or could edit their own posts and this was one of theirs...

I realised that a bit of prep before getting to the "if(..." went a long way. And not a long way to anything in computer terms, but readability -

[quote author="BrianDHall" date="1254256494"]Its really all the human element, you hit the nail on the head there. The problems I find with it are A) lots of nesting isn't pretty and distracts the eye and gives one more thing to have to fix occasionally if your editor gets confused, B) increased chance of mis-matching brackets, putting code in the wrong bracket, or worst of all putting code that assumes a certain security check in the wrong set of brackets, and C) being inside a loop increases human memory usage and requires concentration (you have to constantly remember you are in a loop and what conditions triggered your arrival there) even if it requires no computer over-head in terms of memory or CPU strain.

As ADDey (thats now a word, I just 'precedented' it) as I tend to be, I can't afford any extra mental overhead Wink[/quote]

You're absolutely right. I asked questions about model structure, and ORMS and methods; and then realised that the end result was pretty much the same in terms of functionality. The end result rarely depended on performance (or, as Jedd points out, performance is a minor factor). The end result pretty much depends on "can I, and other developers, follow this code, understand it and modify it without destroying it"? You put it more elegantly - no nested sentences or nuffin.

[quote author="BrianDHall" date="1254256494"]Thanks kurucu, I'm really glad to hear it works as I thought it would as it seems like such a nice option to have in some functions and controllers. Gives me new appreciation for the whole point of pages being 'functions' to begin with - easy to end execution of them without breaking the page or other functions (like exit might do).[/quote]This is why we use the forum!
#6

[eluser]n0xie[/eluser]
I'm just adding my 2 cents.

We've been taught that any function should only have one exit point, which I abide to religiously, because it makes debugging, much much easier. (btw there is also a large group who things you should exit as soon as possible so it's not a silver bullet) The only exception to this rule is precisely your example: exit a function because of a failed precondition. What I mean by that is, code indentation is there to show you the 'paths' of your application. In a sense it is there to show what routes code should take. But these are based on the fact that you actually want the code to execute.

Whenever you have a condition where you do NOT want the code to execute, it's probably easier to exit as soon as possible.

For instance, I always do a sanity check against function parameters (because I don't trust my users and neither should you). If my sanity check fails, there isn't a code path to follow, it just means either something 'unexpected' happened OR something bad happened. Neither is something I like, and in no way would I want to execute any code. Therefor you will find tons of these codeblocks in my code:
Code:
function foo($bar)
{
  if ( ! do_some_sanity_check($bar)) { return/redirect/raise an error/write to log/bake a cake }

  // real code goes here
}
So if something fails my sanity check, the standard action would be to redirect and at the same time write a message to the error log, so I know something happened. The oneliner at the beginning of your function provides an extra exit point but it makes sense. It also doesn't 'indentclutter' your code.

tldr:
I do what you did in your example all the time.
#7

[eluser]John_Betong[/eluser]
 
One way I came across to reduce the else statements was to supply a default value:
[code]

function to_reduce_indentation()
{
$result = '#cfc'; // $alternate bg_colour

if ( (FALSE_CONDITION) )
{
$result = '#cff';
}

return $result;
}
 
And yes I endeavout to have a single exit point since it makes debugging an awful lot easier to temporaily hard-code the return value.
 
 
 
Just noticed there is no spell checker in the "Fast Reply".
 
 




Theme © iAndrew 2016 - Forum software by © MyBB