On Wed, Jul 1, 2009 at 12:42 PM, Sebastian Nowicki<sebnow@gmail.com> wrote:
On Jul 1, 2009, at 7:36 PM, Callan Barrett wrote:
On 02/07/2009, at 1:31 AM, Sebastian Nowicki wrote:
Might be easier to just put the "if(LOGGING) {}" into the logit() function itself, assuming this is the desired behavior for all instances of logging.
I feel like an idiot for not putting it there in the first place but now that I think about it... if logging was turned off we would be doing 2 hits to the DB for almost every single action needlessly if the check was inside the function. So maybe it's not worth the simplicity?
Ah, you mean the calls to username_from_sid() and pkgname_from_id()? If they are always called, you could put that in the function as well, and only pass in the parameters ($_COOKIE['AURSID'], $pid), respectively, etc.). Otherwise you have a good point.
The current interface for the logit function is better (more flexible) than with the above suggested changes though. I'd probably stick with the redundancy.
I'd move the two function calls into logit() as well, as it looks like they are always called. Why are we inserting username when userid should be the foreign key we use to reference users? I guess if you aren't a fan of joins it makes sense, but DB normalization does have benefits, especially when you decided to use integers anyway to represent the different operations. The same holds true for package IDs, why not just use the IDs directly and not names? Getting the same end result table (with usernames and pkgnames) would be only a matter of two simple joins, not hard for a relational DB to do. In addition, magic numbers are not so much fun and make it hard to decipher the calls at first glance. Maybe some sort of associative array could help here, declared near the logit() function? $logit_actions = array( 'upload'=>1, 'update'=>2, ... ); And then a call would end up looking like: logit(logit_actions('upload'), $userID, $packageID); -Dan