Is this too many lines and too many nested blocks?

Question

I have the a function that loads a list of things from a database and put them into a select list. The function is as follows : (pseudocode)

 protected function Foo() 
 {
    try {

        get pdo instance
        prepare statement

        if (pdo query executes) 
        {

            while (row = fetched rows) 
            {
                do stuff with row
            }
        } 
     } 
     catch (PDOException $ex) 
     {
         do error stuff here
     }        
 }

NetBeans gives a code hint that it is too many lines and too many nested blocks. I personally feel that the function should be acceptable. I also feel that to break the logic up into smaller functions is bit looney, but why would netbeans lie to me:) ?

so my questions are as follows:

Is this bad logic or am I good to go ahead? I would welcome any suggestions on how I might redesign the function to fit within the NetBean constraints.

edit:

I won't answer my own question but in this case there was one nested block that was not needed. The pdo is retrieved from a singleton class that has the try/catch block. I dont need to repeat it again in this function because the exception would of already been caught.

edit 2:

Removing the try catch block was just like robbing Peter to pay Paul. So if the exception is thrown in the creation of the pdo instance it does not stop execution. Thus, we try call the prepare statement on a PDO object that was not initialized correctly. This forces us to put in another test before the prepare call, thus just returning to a rework of the original function.

In my experience, this means somewhere along the line my logic is floored. I am going to go over my design and holla back if i have anything worthy to say.

Thanks again to all

Truth be told, it is 4 lines that actually perform an action in a 5 levels deep code, but that shouldn't be too bad. Just try writing some more lines of code and it should go away (or disable the function in Netbeans). — Erik S, May 12 '15 at 09:49
The logic looks fine. Generally as a pattern you might want to abstract the setting up of the PDO instance into its own class with its own try/catch/error reporting architecture. Then you can just grab the resulting class-level pointer to the PDO instance and assume in the rest of your code you don't need to wrap everything in try/catch, and you can just check whether the query executed or not. Going through the rows is fine, although (not sure what language) probably has a fetchAll type function that dumps it into a neat little array for you. — joshstrike, May 12 '15 at 09:50
Thanks guys i figured as much!! I do have the PDO instance in a singleton class and in a try catch block as well. The reason why I had the while for going into the array is each result is being put in to another object for example - myObject->InsertChild(row); if you get my drift. thank you Erik and Josh for your guidance — Shibby, May 12 '15 at 09:54

Sarfraz · Accepted Answer · 2015-05-29T12:13:04.513

Your code is good. What NetBeans suggests is not necessarily a rule you should follow and you wouldn't even have worried if you used some other editor like PHPStorm (In PHPStorm, you can set coding style to follow PSR 1/2).

You could at least do away with one nesting by using something called guard clause:

protected function Foo() 
{
    try {
        get pdo instance
        prepare statement

        if (! pdo query executes) return; 

        while (row = fetched rows) 
        {
            do stuff with row
        }

     } 
     catch (PDOException $ex) 
     {
         do error stuff here
     }        
}

I have seen people having different preferences since there is no hard and fast rule. For example Anthony Ferrara personally thinks he should not go beyond four nested levels where I personally think four levels are too much.

Point is you should minimize nesting and number of lines as much as you can. When your method is too big (sometimes called god method), it means you are doing it wrong.

You may also want to look at this nice article by William Durand on the subject where he discuss few (actually 9) proposals suggested by Jeff Bay in his book ThoughtWorks Anthology

Object Calisthenics

Also PHP Coding Standards Fixer is your friend.

So:

Your current code is perfectly fine
Create your personal preference and minimize nesting and number of lines where possible.

Thanks you for that. I will take a look at those articles and follow your recommendation. I try to keep all my classes and functions minimal but complete. thanks once again, all the best — Shibby, May 12 '15 at 12:03

score 0 · Answer 2 · answered May 12 '15 at 09:57

0

You could reverse the if statement and exit the function if the condition is not true to eliminate one level of nesting. The logic looks OK too me though.

answered May 12 '15 at 09:57

Nicholas J. Markkula

1,542
4
18
30

Thanks for your comment. I have just removed the redundant try/catch block as i already have a try catch block when i create the instance of the POD object. all the best – Shibby May 12 '15 at 10:02
Hi, thanks for your help. i marked the answer below just because it covered a bit more of the general theory but same coding solution as what you said. Thanks once again – Shibby May 12 '15 at 12:02

Is this too many lines and too many nested blocks?

2 Answers2