Style questions

izzion

Because I know that future me is going to wind up in an argument with current me over whether or not this code format is a good idea, I thought I'd seek ~~expert help~~ a few dozen troll opinions over my formatting...

I've got a project that's taking REST-style requests to return/modify information about our inventory & service orders. The API backend is PHP, making use of spl_autoload and the like to let me separate my structures. General flow basically goes:

Request comes in, master file looks at URI to determine what type of request this is, constructs a "parser" object of the appropriate class (apiCustomer, apiItem, etc)
Master file calls $parserClass->processRequest and passes the request to the class's handler. The class handler reads through the request, validates/authenticates input, and then does whatever is necessary (read, write, etc ops) to prepare the response back.
Within the database classes, my general structure is the different functions (getCustomer, updateCustomer, deleteCustomer, etc) as public static functions that receive the necessary parameters and then go talk to the database.

Many of my "read" operations have the potential to be called multiple times in a single request (such as, get the entire item list so we can start an inventory adjustment). In this case, my DB class is set up roughly as:

private static $getItemInfo;
public static function getItemInfo($itemID) {
    if ( is_null(self::$getItemList) ) {
        $c = DBClass::connect();
        self::$getItemList = $c->prepare(*statement*);
    }
    $q = self::$getItemList;
    $q->bindValue(":id",$itemID);
    $q->execute();
    return $q->fetch(\PDO::FETCH_ASSOC);
}

Does it make sense to do the "private variable" as a holder for lazy-initializing the database prepared statements? SQL Server Profiler definitely showed that when I just initialized the prepared statement within the getItemList() function, it gets set up and torn down on every single call to that function (which was a duh moment when I saw it). So I switched to this method, but I kind of feel like it's obtuse/there's a better way to do it...
PHPStorm's type hinting and rudimentary testing suggest changing the prepare statement to:

self::$getItemList = DBClass::connect()->prepare(statement)

will work. Is there any reason, from a longer term maintainability perspective, to prefer one method over the other? The one-liner seems more compact to me, but I'm split on whether "more compact" means "better" or "more obscured". Is there a performance reason not to use $c as an intervening step? I'm guessing the habit is something I have from back in the way old days when I read a webguide somewhere telling me to do $c = mysql_connect(connstring) and then mysql_query($c,query) and all, but I'm kind of split on whether it's still more readable in the PDO days.

TIA for being the deciding vote on my split internal council :P

RaceProUK

The single-line is fine as far as I'm concerned.

reverendryan

@izzion, post:1, topic:8466, full:false said:

self::$getItemList = DBClass::connect()->prepare(*statement*)

That's the pattern I follow too. The only reason to keep $c around would be if you're going to do another query and DBClass doesn't keep the same connection open between calls to connect().

cartman82

This looks like a lot of code repetition if you use it in every method. Isn't there a way to abstract some of this away? Perhaps by using some higher level abstraction library?

Sorry I can't be more specific, I'm rusty with PHP. This just feels "smelly".

izzion

DBClass::connect() should probably be renamed to DBClass::getConnection()... it's basically the getter for the private static variable that holds the connection open for the duration. I suppose I could make use of that wonderful "refactor" thingy-wingy in my IDE

reverendryan

@cartman82 said:

Isn't there a way to abstract some of this away? Perhaps by using some higher level abstraction library?

Yeah, that looks pretty generic and could probably be moved into a base class pretty easily. I assume the SQL looks something like SELECT * from table WHERE id=:id, and the only thing that is different between classes is the name of the table.

izzion

I've actually tried to be a fairly good boy and not SELECT * in cases where I don't need it

Some (maybe 10%?) of my queries are such that the query string is built "at request time", based on what search parameters the front end user specified, or whatever. Most of them are pretty static, with the parameters determining what values are searched for in the WHERE clause or what have you. I did the query-within-the-db-class-functions setup mostly because that's how I think about it (rather than coming at it from a sproc angle or what have you). Some of the queries get to be a bit fugly though (multiple UNIONs with multiple JOINs in each query), but most of them are pretty generic.

In general, my setup is:
DBClass - has a method "getConnection" that returns the PDO connection for the query classes to use.
QueryClass (Customer, Item, etc) - has a series of methods for operations on a specific thing ("customers", "items", "service orders") - create, retrieve, update, delete, etc. Some classes have multiple different types of retrieve methods (getCustomer, getCustomerServiceAddress, searchCustomer), depending on what sort of requests the front end application can make. Within each QueryClass method, the general flow is grab the prepared statement for that method (QueryClass::$getCustomer), bind the input parameters to it, then execute the query and return the results back to the API handler that requested the query. The handler then turns the results into JSON to send back to the front end application.

If it could be better extracted so that future-me (or the replacement who gets to work on this when I get hit by a bus) can maintain it better, I'm all about learning a better way to do this. Most of my coding knowledge (ok, all of it) is the result of Google searching and self-teaching... I'm a finance major who knows how computers work as a hobby and turned a desktop support job into programming a PHP Web Application to extend our billing system

boomzilla

@izzion said:

Does it make sense to do the "private variable" as a holder for lazy-initializing the database prepared statements?

Have you gathered some profiling data to see if it matters? I use Oracle, so YMMV, but I've never encountered anything where the compilation of queries was a significant factor in performance.

Jarry

self::$getItemList = DBClass::connect()->prepare(statement)

unless you need $c later.

you are building an ORM?
related reading:

PHP: The Right Way

An easy-to-read, quick reference for PHP best practices, accepted coding standards, and links to authoritative PHP tutorials around the Web

@boomzilla said:

but I've never encountered anything where the compilation of queries was a significant factor in performance.

same here. and, if you aren't going to crunch a lot of data. your performance depends on other factors almost always

PleegWat

Oracle's prepares aren't DB round trips so they're pretty cheap.

izzion

Well, the worst query at this point is the item list, which takes about 3-4 seconds to return if the primary database is active, and 15+ if the secondary database is active. My recollection was that (MS SQL 2014, using an AAG) SQL profiler showed a not-insignificant amount of time on that query was spent setting up & tearing down the prepared statements, especially on the secondary database.

Given that the two database VMs are supposed to be clones (except for one running on VMWare and one running on Linux KVM for the underlying hypervisor), I suppose the big query time difference is my better thing to chase down... but I was hoping that lazy-initializing a single prepared statement, instead of re-preparing the statement 600+ times, might give me a low-hanging fruit optimization.

Edited: Well, damn, I'm an idiot. Looks like @boomzilla is right, the performance isn't actually the preparing, it's executing the query. SQL Profiler just doesn't actually pull that out into a separate line, so newbie not-a-DBA me didn't realize that the "10 reads" includes preparing the statement AND executing it. And testing shows that executing the previously prepared statement costs... "10 reads".

boomzilla

@izzion said:

but I was hoping that lazy-initializing a single prepared statement, instead of re-preparing the statement 600+ times, might give me a low-hanging fruit optimization.

If it were me, I'd probably try it to see what happened, but...

@izzion said:

the big query time

I'd look for benefits here. Even 3-4 seconds sounds like a lot.

@izzion said:

600+ times

Like, for a single request? The round trips alone are probably killing you. If that's the case, I'd look to combining some of those.

@PleegWat said:

Oracle's prepares aren't DB round trips so they're pretty cheap.

What about SQL Server? Huh...looks like it's a round trip.

izzion

My edit crossed your reply, but yeah, I'm in the middle of trying to see what happened, and trying to combine a dose of "does this style even make sense" along with performance testing. Multi-tasking and all. And I guess the performance isn't any better with the "static variable holding the prepared statement" method, so I'm gonna just whack that in the knees.

izzion

Well, tuning my queries to pull both of the "subquery" things I was doing per item into the master item list query seems to have cut my development environment execution time from 1300-1700ms to 500-600ms. Progress \o/

izzion

Hm. Apparently somewhere in the process of converting my structure to use spl_autoload, the PDO connection stopped throwing exceptions.

self::$dbConn = new \PDO("sqlsrv:Server=".$hostname.";multiSubnetFailover=yes;", $username, $password);
self::$dbConn->setAttribute(\PDO::ATTR_ERRMODE, \PDO::ERRMODE_EXCEPTION);

In the case where I'm having problems, I've retrieved the $dbConn handle into a local variable for the function using it. Then if I try to print the value of ATTR_ERRMODE, it outputs 2, which seems to my "read the documentation" eye to be "throw exceptions". But my catch isn't catching a \PDOException

$c = PlatDB::getConnection();
print_r($c->getAttribute(\PDO::ATTR_ERRMODE))

Anyone have an idea why my catch statement broke? Nothing else different between production and development for this function, except that development is set up for spl_autoload and thus has these functions inside a namespace. Is @arantor still lurking and enough of a PDO/PHP Namespaces expert to bail me out?

Edit: for clarity -- yes, my catch block is looking for \PDOException.

Edit 2: And yet, if I put a really simple stupid "SELECT notacolumn FROM notatable" statement ahead of the executed prepared statement that is failing (due to a PK violation), the PDOException from my testing error does, in fact, get caught as expected. I officially hate my life.

Edit 3: Seems to be that PK violations aren't throwing PDOExceptions -- or outputing any error at all, even with display_errors On -- just failing with Server Error 500 -- when my query is run in the development environment (as far as I can see, the only difference on these queries is that the development environment is namespaced and using spl_autoload to call up the functions I'm using). Non-PK SQL errors are still throwing as expected/as in production. Both servers are IIS 8 + PHP 5.3.28 + Microsoft SQL Drivers 3.0 for PHP + the same connection string to PDO.

Edit 4: And, just for the lulz (and so I would have 2 problems), I ~~used a regular expression~~ upgraded to PHP 5.5.11 on the dev server to see if that would change the behavior. It did not. So then I un-did the namespace for the database functions class (but not the "database connections" wrapper), and it worked. Dammit...

Edit 5: And then after I reverted my testing with git and retested, it broke, but I wasn't paying enough attention, because it broke because I reverted the web.config file for the API that had a modification to the handler, so it put the handler back to PHP5.3 (which is no longer active on the server) instead of PHP5.5. So I fixed that (dammit, I need more caffiene) and now the PDOException for the PK violation is getting caught. I hate my life.

This post is deleted!