4
\$\begingroup\$

In my never-ending quest to improve my coding skills I am requesting peer input on the following code logic. I am asking because I'm -NOT- an expert, but would like to be one day. Source material and recommended read is greatly welcome (no, do NOT point me to the PHP manual) especially for code formatting, unit testing, etc.

Notes:

  1. Layer separation is not important; i.e.: presentation and database connection logic was not abstracted to different classes. This is an admin usage only script. Sporadic usage, saw once a month.

  2. I have just learned about dependence injection; was attempting to use it properly.

  3. PHP version is 5.3.3. so fancy stuff like mysqli::query_all(ASSOC) is not an option.

Please let me know, in a constructive way please, how I could improve my skill at this trade.

<?php

//script settings. Prevents timeouts
set_time_limit(0);
ignore_user_abort(1);

//constants
const DEBUG = false;
const EXECU = true;

/**
*@Comment: Create all the reused and repeated methods.
*/
class DependencyContainer
{
/**
* @Param: $location string [required], $user string [required], $password string [optional], $database strong [required]
* @Return: $mysqliConn object
*/
public static function dbConn( $location = null, $user = null, $password = null, $database = null )
{
    //Inbound var check
    if($location == null || $user == null || $database == null)
    {
        //public function status($title = null, $response = null, $die = 0)
        $this->status("createDBObject missing params", "ERROR", 1);
    }

    //Connect me to the SugarCRM database
    $mysqliConn = null;
    try
    {
        $mysqliConn = new mysqli($location, $user, $password, $database);
    } catch (Exception $e) {
        $this->status('Could not create DB object', $e, 1);
    } 

    //if connection fails
    if ( $mysqliConn->connect_error )
    {
        DependencyContainer::status( 'Could not connect to host', $mysqliConn->connect_error, 1 );
    }else{
        return $mysqliConn;
    }
}

/**
* @Comment: Debug function call./
* @Param: title string, response string, die int
*/
public static function status( $title = null, $response = null, $die = 0 )
{            
    //no debug for you!
    if( DEBUG == false )
        return;

    $isBrwsr = true;
    //determine if we are in a browser or CLI
    //spefcial thanks to: http://www.codediesel.com/php/quick-way-to-determine-if-php-is-running-at-the-command-line/
    if(php_sapi_name() == 'cli' && empty($_SERVER['REMOTE_ADDR'])) {
        $isBrwsr = false;
    }

    //easy full stop AND if debug is off and an error occures.
    if($title == null)
        $this->dieRequest("An error has occured. Please contact your administrator.", 1);

    if($isBrwsr)
        echo("<pre><div color='black'>");    

    echo "\n" . $title.": ";

    /**
    * @Comment: should make this check for true or 1 as well to be a positive response.
    */

    if ($response != null && is_numeric($response) == false)
    {
        if($isBrwsr)
            echo("<div style='color:red;font-weight:bolder;display:inline;'>");

        print_r($response);

        if($isBrwsr)
            echo("</div>");
    }
    elseif ($response == null)
    {
        if($isBrwsr)
            echo("<div style='color:green;display:inline;'>");

        print_r("GOOD or null");

        if($isBrwsr)
            echo("</div>");
    }
    elseif (is_numeric($response) == true)
    {
        if($isBrwsr)
            echo("<div style='color:green;display:inline;'>");

        print_r($response);

        if($isBrwsr)
            echo("</div>");
    }                

    if($isBrwsr)
        echo("</div></pre>");        

    if($die != 0)
    {
        self::dieRequest();
    }
}

/**
* @Comment: Le-killer functions! No srsly, its a killer!
* @Param: callReferance, showDie int
*/
public static function dieRequest( $callReferance = null, $showDie = 0 )
{
    if($showDie != 0)
        echo("Die requested by: " . $callReferance);

    die;
}

/**
* @Commentr UUIDv3 generator
*/
public static function UUIDmaker()
{
    return sprintf( '%04x%04x-%04x-%04x-%04x-%04x%04x%04x',
        // 32 bits for "time_low"
        mt_rand( 0, 0xffff ), mt_rand( 0, 0xffff ),

        // 16 bits for "time_mid"
        mt_rand( 0, 0xffff ),

        // 16 bits for "time_hi_and_version",
        // four most significant bits holds version number 4
        mt_rand( 0, 0x0fff ) | 0x4000,

        // 16 bits, 8 bits for "clk_seq_hi_res",
        // 8 bits for "clk_seq_low",
        // two most significant bits holds zero and one for variant DCE1.1
        mt_rand( 0, 0x3fff ) | 0x8000,

        // 48 bits for "node"
        mt_rand( 0, 0xffff ), mt_rand( 0, 0xffff ), mt_rand( 0, 0xffff )
    );
}
}

class Accounts
{
//class properties
private $mysqliConn;

/**
*@Comment: create the object. All aparams are as an array
*@Param $dbDependency object [required]
*/
function __construct($dbDependency = null)
{
    if( $dbDependency != null )
    {
        $this->mysqliConn = $dbDependency;
    }   
}

/**
*@Comment Get all Accounts id, phone_office, phone_alternate
*@Param n/a
*@Return $dataArray 
*/  
public function getAccountsData( $columns = null, $limit = null )
{

    if( $columns == null )
    {
        //for now all we check against is the phone_office data
        //$columns = "`phone_office`, `phone_alternate`";
        $columns = "`phone_office`";
    }

    //create SQL query    
    $stmt = "
        SELECT ". $columns ."
        FROM `accounts`
        WHERE 1
        ". $limit . ";"
    ;

    //exec query
    $query = $this->mysqliConn->query($stmt);

    //coallate results
    $getAccountsData = array();
    while ($row = $query->fetch_assoc())
    {
        foreach( $row as $rowKey=>$rowValue )
        {
            if( stristr( $rowKey, "phone" ) &&  $rowValue != "" )
            {
                $getAccountsData[] = $rowValue;
            }
        }
    }

    //DependencyContainer::status('$getAccountsData', $getAccountsData);
    return $getAccountsData;    
}

}

class Leads
{
//class properties
private $mysqliConn;

/**
*@Comment: create the object. All aparams are as an array
*@Param $dbDependency object [required]
*/
function __construct( $dbDependency = null )
{
    if( $dbDependency != null )
    {
        $this->mysqliConn = $dbDependency;
    }   
}

/**
*@Comment Get all Accounts id, phone_office, phone_alternate
*@Param n/a
*@Return $dataArray 
*/  
public function getLeadsData( $columns = null,  $limit = null )
{

    if( $columns == null )
    {
        $columns = "`phone_home`, `phone_mobile`, `phone_work`, `phone_other`, `phone_fax`, `assistant_phone`";
    }

    //create SQL query    
    $stmt = "
        SELECT ". $columns."
        FROM `leads`
        WHERE 1
        ". $limit . ";"
    ;

    //exec query
    $query = $this->mysqliConn->query($stmt);

    //coallate results
    $getLeadsData = array();

    while ($row = $query->fetch_assoc())
    {
        foreach( $row as $rowKey=>$rowValue )
        {
            if( stristr( $rowKey, "phone" ) &&  $rowValue != "" )
            {
                $getLeadsData[] = $rowValue;
            }
        }
    }

    //DependencyContainer::status('$getLeadsData', $getLeadsData);

    return $getLeadsData;
}

/**
*@Comment Create a lead based on an array of phone numbers
*@Param $data array [required]
*/
//this is terribad, but works for now
public function createLead( $data = null )
{

    DependencyContainer::status('Accounts to create leads for', count( $data ) );   

    $insertStmt = "";
    $stmt = "";
    $counter = 1;

    foreach( $data as $dataKey=>$dataValue )
    {   

        DependencyContainer::status('$counter', $counter);      
        //SELECT from Accounts
        //Get account associated with the phone_office number
        $stmt = "SELECT `id`, `phone_office`, `name`, `assigned_user_id` FROM `accounts` WHERE `phone_office` = '". $dataValue . "';";

        //exec query
        $row = $this->mysqliConn->query($stmt);

        $relatedAccountData = $row->fetch_assoc();

        //INSERT into LEAD
        //query statment
        $insertStmt .= "INSERT
            INTO `leads`
            (`id`, `assigned_user_id`, `phone_work`, `status`, `account_name`)
            VALUES
            ('". DependencyContainer::UUIDmaker() ."', '". $relatedAccountData['assigned_user_id'] ."', '". $relatedAccountData['phone_office'] ."', 'Converted', '". $relatedAccountData['name'] ."');
        ";

        $relatedAccountData = "";

        //abrstract to method
        //error catching
        try
        {
            DependencyContainer::status('$insertStmt', $insertStmt);

            //testing
            if( EXECU == true && $this->mysqliConn->query($insertStmt) )
            {
                DependencyContainer::status('SQL INSERT  executed');
            }elseif( EXECU == false ){
                DependencyContainer::status('-TEST- SQL INSERT executed.');
            }else{
                DependencyContainer::status('SQL INSERT DID NOT execute.', $this->mysqliConn->error);
            }
        //caught
        } catch (Exception $e) {
            //if things go south
            DependencyContainer::status('Exception', $e, 1);
        }

        $insertStmt = "";
        $counter++;
    }

    return true;
}
}

class Comparison
{
public static function comparData( $arrayFrom = null, $arrayAgainst = null)
{
    //Logic from merlyn dot tgz at gmail dot com 15-Mar-2012 10:08 @  http://php.net/manual/en/function.array-diff.php
    foreach( $arrayFrom as $key => $value )
    {
        //check for number matches
        if( is_numeric( $value ) )
        {


            if( in_array( $value, $arrayAgainst ) )
            {
                unset( $arrayFrom[$key] );
            }
        //remove non-numeric keys and values
        }else{
            unset( $arrayFrom[$key] );
        }
    }

    //From array minus Against array
    //exp: 9169286000
    DependencyContainer::status('$arrayFrom after removals', $arrayFrom, 1);
    return $arrayFrom;
}
}

//auto password :)
$pw = "";
if( isset($_SERVER['HTTP_HOST'] ) && $_SERVER['HTTP_HOST'] == "localhost" )
{
$pw = "";
}else{
$pw = "***passwordHere***";
}

$dbDependency = DependencyContainer::dbConn("localhost", "root", $pw, "sugarcrm");

$accountsClass = new Accounts($dbDependency);
$accountNumbers = $accountsClass->getAccountsData();

$leadsClass = new Leads($dbDependency);
$leadNumbers = $leadsClass->getLeadsData();

$results = Comparison::comparData( $accountNumbers, $leadNumbers );

$leadsClass->createLead( $results );
\$\endgroup\$
  • 2
    \$\begingroup\$ Its not, but its also unnecessary. All this does is bring it back to the top of the general list. It is already at the top of the PHP list. Just be patient. This community is a bit slower than the other SE communities. That being said, sorry I didn't get around to my review sooner, my Raspberry Pi arrived last night and I was really excited to start playing with it. \$\endgroup\$ – mseancole Sep 27 '12 at 16:13
  • \$\begingroup\$ General Review and First DI Attempt would be a good one \$\endgroup\$ – mseancole Sep 27 '12 at 16:56
4
\$\begingroup\$

Well, first suggestion, don't use set_time_limit(). Try and refactor your code so that it is unnecessary, unless this is a CRON job or something and is running in the background. If you access it via a webpage then even if you are expecting it to take a while, it could be confusing. For instance, if you call this script via AJAX, then you will not see that it is loading, it will just sit there appearing to be doing nothing. I ran into a similar problem with a couple of my AJAX scripts recently and had to figure out how to separate the results so it didn't take as long. Additionally, be careful with that ignore_user_abort() function as it will require you to have to restart your server should you ever accidentally trigger an infinite loop.

You got your constants confused. define() is used to create a constant outside of a class. const is used to define a constant inside of a class.

No need to explicitly declare @Comment, just start typing. Your IDE should know that its a doccomment and act accordingly. @Comment isn't even a PHPDoc tag. See this link for a list of available tags and their uses. Its also important to note that if you give everything a decently descriptive name, doccomments like this are unnecessary. They should really only be used to point out the caveats and anything you think wont be obvious just by looking at the code.

When using @param you should only give one parameter. Declare new @params for each new parameter, otherwise you won't get the proper highlighting. Also, you have your parameter type and name switched.

/**
 * @param String $location etc...
 * @param String $user etc...
 etc...
 */

What is the point of all of these static methods? This completely defies the point of even having a class. You mentioned suggested reading, here's a wikipedia entry for OOP. I would suggest reading the whole thing, but that first paragraph is the important one here. In it it points out the key concepts of OOP. I would also suggest following the links to each of those concepts for a better understanding. Once you have done this, come back and look at your code and ask yourself if you are following most of these concepts. You don't have to follow every one of them, but if you aren't at least following most, then you have not justified OOP. In short, you probably wont need static very much at all. I've been doing this for quite a while and everything I have started to do with static I ended up scraping or turning into a helper function. Speaking of which, don't be shy about using helper functions either. If it doesn't fit in a class don't try and force it, helper functions are still useful, even in an OOP context.

You already have doccomments, why the inner comments? If you really need to document something about the method, do so in the doccomments at the beginning. That's what they are there for. Leave the inside of your methods comment free so that they don't get cluttered. Believe it or not, this really helps when trying to read the code, you wouldn't believe the difference it would make, especially if you auto-collapse doccomments in your IDE.

You should either use absolute equality === comparisons when checking for NULL, or the is_null() function. Personally, I prefer the later approach, but there is no real difference between the two, see this post for more. Unless, of course, you are not just checking for NULL, but for any FALSE value, in which case you can just do the following.

if( ! $location || ! $user || ! $database ) {

I'm actually kind of surprised this works at all... $this is not static and therefore can not be called in a static context. Use self:: instead. See this tutorial for details. I see you did this, or something very similar towards the end of the method, but the non-static implementation is still prevalent in the rest of your code and is wrong. Of course, removing the static keyword would be ideal and would make this unnecessary.

First, before going on, let me just say that I am seeing quite a few inconsistencies in this code: spaces between parenthesis, no spaces; switching back and forth between single and double quotes; $this-> vs. DependencyContainer:: or self::, as I mentioned above. This leads me to believe you either copy-pasted this together, have multiple people working on it, or have changed your style and not retroactively gone back to change everything. If one of the last two, strive to at least make each file consistent. If its too much work to go back and change everything, adjust your current style to fit. Or if you are working in a group, have a meeting and discuss code style and make everyone adhere to it. If its the first one, don't ever do that. Always type out any code you borrow. Doing so may help you to better understand it, or at the very least will ensure that its not obvious you stole it :)

But, the most heinous of all, IMO, are these braceless statements, especially when you use braced single statements as well. PHP inherently requires braces, otherwise you wouldn't have to add them after extending a statement past the first line. As such, I don't think braceless statements should be allowed at all. If they ever change the code so that those braces are entirely optional, then I will retract this statement, but in the meantime, I think this is the wrong way and I am going to stand by that. But either way, again, be consistent.

if( DEBUG == false )
    return;
//OR
if( DEBUG == FALSE ) {
    return;
}

I mentioned this before, but it was in another context, so I'll repeat it here for clarity. If you are not using an absolute equality comparison, then you don't have to explicitly compare the variable to a boolean. Just treat it as a boolean, it does the same thing.

if( ! DEBUG ) {

Here's another inconsistency. Your echo statements do not have to have parenthesis, but you should pick one method and stick with it. I also mentioned something about quotes above. To elaborate, use single quotes when the string you are writing does not have any escape sequences, or if you need to use double quotes. Use double quotes for the opposite. And you can use your preference if the type isn't important, though I will say that single quotes are very slightly faster because PHP knows it doesn't need to escape anything. Additionally, you should use double quotes for your HTML attributes, so for those HTML strings I would use single quotes. If this string were to be loaded into DOM or treated as XML, then this would throw some errors. And finally, I mentioned using double quotes for escape sequences above, this also includes variables. The following echo statements are a little better, though the last one could be argued. Some don't like using variables in string sequences and instead concatenate it, up to you, I just find this cleaner.

echo '<pre><div color="black">';
echo "\n$title: ";

The following is just begging to be refactored. First, it violates "Don't Repeat Yourself" (DRY) Principle, second, you don't want to have to check the same statement more than once. You can combine these by checking $isBrwsr at the very beginning and setting two variables, initially empty strings, maybe $header and $footer with a generic opening and closing div tags respectively. Then, wrapping these variables, whether empty or not, around the context variable will accomplish the same thing. If you do it right, you can use a placeholder in the $header so that you can then use the context, set up by your if/else statements, to set the div's class or id with a sprintf() function or substr_replace(). Then, using a stylesheet, you can save those individual styles and only call the one you need via class or id. Additionally, print_r(), unless using an array or object here, is unnecessary. echo is just fine, and var_dump() is more informative if just being used for debugging. I would show you how to do this, but I believe the above explanation should be adequate. If you need help, show me what you have tried and I'll take a look.

if($isBrwsr)
    echo("<div style='color:red;font-weight:bolder;display:inline;'>");

print_r($response);

if($isBrwsr)
    echo("</div>");

I'm not going to dissect that UUIDmaker(), because I'm not sure exactly what you are doing here or why, but I'm fairly certain this could be refactored somehow.

I kind of started skimming here. A lot of the issues I was finding were repeats, and there is a lot to look over. So take what I said above, and apply it to the rest. Something I didn't go over in the above review was Single Responsibility Principle, but you could stand to look into that as well. Here's a good starting place, and don't be shy about reading the related material either. One of those should be related to DRY, which I mentioned above.

I'm not going to go too in depth for the Dependency Injection as there is already quite a bit here and this would only add a considerable bit more, but here are a few things to keep in mind. Dependency Injection is typically not optional if being implemented, so if it is null it either creates a new instance or fails. Dependency Injection requires objects of a specific type, so declaring that type by type-hinting your parameters will help quite a bit. And most importantly of all, Dependency Injection should only be done in a class constructor. Dependencies should not be passed in at any other point.

public function __construct( DependencyContainer $dbDependency ) {

Additionally, as shown above, always declare your method's or property's access type. By default it is public, but I have heard rumors of this being deprecated in the future. Don't rely on the language defaults to save you. Besides being specific about this kind of thing is important. Being in the habit of neglecting this could lead to mistakes, such as forgetting to make a method or property private, for example, a password.

As I said, I'm skimming at this point, but this is the last thing that jumped out at me. If you are not going to use the key, you do not need to define it in your foreach statements.

foreach( $data as $dataKey=>$dataValue ) {
//remove the key
foreach( $data as $dataValue ) {

I'm sure there are a few things I missed, but this should definitely help you get started.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.