Please review my php code (It's report from order table)

Question

<?

$debug = (isset($_GET['debug'])  && !empty($_GET['debug']))? $_GET['debug'] : false;

define('TYPE_A', 'A');
define('TYPE_O', 'O');
define('DEBUG_STAT', $debug);        

//get params from $_GET or setup default value
$payment_type = ((isset($_GET['payment'])  && !empty($_GET['payment'])))? $_GET['payment'] : 'fghue';
$currency     = ((isset($_GET['currency']) && !empty($_GET['currency'])))? $_GET['currency'] : 'GBP';
$type         = ((isset($_GET['type'])     && !empty($_GET['type'])))? $_GET['type'] : 'O'; 

//get site id by site name
$site_filter      = array('siteone' => 30, 'site2' => 9, 'site3' => 12 );    
$site_name        = (isset($_GET['site']) && !empty($_GET['site']))? $_GET['site'] : 'site1';
$site_id          = (array_key_exists($site_name, $site_filter)) ? $site_filter[$site_name] :  $site_filter['siteone'];

//select statistic from period
$period_time      = array(
    'start' => '2012-01-01',
    'end'   => '2012-03-31'
);


ShowAdmHeader();

echo ("ver 1.4");

if ( defined('DEBUG_STAT') && TRUE == DEBUG_STAT ) { 
    echo "<p>" . $payment_type . "/" . $currency . "/" .  $site_id . "/" .  $site_name . "/" . $type ."</p>";
}

$sql = new SQL();
$dbc = new MYDB();

if(defined(DEBUG_STAT) === true)
    echo "<p>Create params array</p>";

//Data to create query     
$params = array(
    'payment'   => $payment_type,
    'currency'  => $currency,
    'type'      => $type,
    'site'      => $site_name
);

if(DEBUG_STAT)
    echo "<p>Try to get order message</p>";

$res1   = get_order_message($sql, $dbc, $params, $site_id, $period_time);

if(!is_array($res1) || empty($res1)) {        
    trigger_error(__FUNCTION__.': Order message array is empty ', E_USER_WARNING);         
}

$res2   = get_order_date($sql, $dbc, $params, $site_id, $period_time);

if(!is_array($res2) || empty($res2)) {        
    trigger_error(__FUNCTION__.': Order date array is empty ', E_USER_WARNING);         
}     

$explanations = array();
$output       = array();

foreach ($res1 as $r)
{ 
    $period = $r['period'];
    $msg    = $r['msg'];
    $cnt    = $r['cnt'];

    if ($r['CHARGED'] || $r['REFUNDED'])
    {
        $msg = 'Approved';
        $cnt = intval($r['CHARGED']) + intval($r['REFUNDED']);
    }
    elseif (!$msg) continue;

    if (strpos($msg,"TRANSID=")===0) $msg = "{fraud filter}";

    $index = $explanations[$msg];
    if (!$index) 
    {
        if ($msg) $explanations[$msg] = count($explanations)+1;
        $index = $explanations[$msg];
    }

    if ($data[$period][$index]) $data[$period][$index] += $cnt;
                           else $data[$period][$index] = $cnt;
}

$explanations['other'] = count($explanations) + 1;
$index = $explanations['other'];


foreach ($res2 as $r)
{
    $period = $r['period'];
    $cnt    = $r['cnt'];

    $data[$period][$index] = $cnt;
}

$cnt = count($explanations);

$type_filter = array('A', 'O');
$out = get_filter_line($type_filter, $params, 'type');
print_out($out);

$out = get_filter_line(array_flip($site_filter), $params, 'site');
print_out($out);

$payment_filter = array('dkijhfw','fghue','sdgsdg','payspace');       
$out = get_filter_line($payment_filter, $params, 'payment');
print_out($out);    

$currency_filter = array('GBP','USD','CAD','AUD','EUR','INR','ZAR');
$out = get_filter_line($currency_filter, $params, 'currency');
print_out($out);


echo '<hr>';

echo '<table><tr><td valign=top>';

    echo "<table border='1'>";
    echo "<tr><td>&nbsp;</td>";
    for ($i=1;$i<$cnt;$i++)
    {
       echo "<td>A<small>".($i)."</small></td>";
    }

    echo "</tr>";

    foreach ($data as $k=>$r)
    {
        echo "<tr><td><b>".$k."</td>";
        for ($i=1;$i<$cnt;$i++)
        {
           if ($r[$i]) 
           {
               if ($i==1) $color = '<font color="green">';
                     else $color = '';
               echo "<td>".$color.$r[$i]."</td>";

               if ($i > 1 && $i<$cnt) $r[$cnt] = $r[$cnt] - $r[$i];
           }
           else echo "<td>&nbsp;</td>";
        }
        echo "</tr>";
    }
    echo '</table>';

echo '</td><td valign="top">';

    foreach ($explanations as $k=>$v)
    {
        if ($v<$cnt) echo "<nobr>A<small>".$v."</small> - ".$k."</nobr><br>";
    }

echo '</td></tr></table>';

ShowAdmFooter();



function get_filter_line(array $data, $params_get, $type ) {

    $out = null;
    //save current position value
    $current_position = $params_get[$type];

    if(empty($data)) {
        trigger_error(__FUNCTION__.': Parameter $data is empty', E_USER_WARNING); 
        return false;                
    }       

    foreach ($data as $value)
    {   
        $params_get[$type] = $value;

        $url = http_build_query($params_get, '', '&'); 

        if ($current_position == $value)
        {
            //don't create link to current position 
            $out .= "<b style='margin-left:20px;'>{$value}</b>";
        } else {
            $out .= "<a style='margin-left:20px;' href=?{$url}>{$value}</a>";
        }
    }

    return $out;
}

function print_out($data) {

    if(empty($data))
        return;

    echo "<div style='margin: 5px 0 5px 0;'>";
    echo $data;
    echo "</div>";

}

function get_url_params(array $params_get) {

    pr($params_get);

    $param = array();

    foreach ($params_get as $param_name => $param_val) {

        if(empty($param_val)) {
            trigger_error(__FUNCTION__.": Parameter $param_name is empty", E_USER_WARNING);                
        }

        $param[$param_name] = $param_val;
    }

    pr($param);

    return http_build_query($param, '', '&');
}

function get_order_date(SQL $sql, DatingDB $dbc, array $params, $site_id, array $period) {

    if(!isset($params['type']) || empty($params['type']) ) {            
        trigger_error(__FUNCTION__.": Parameter type in params is empty", E_USER_WARNING);
        return false;
    }

    switch($params['type']) {
        case TYPE_A:
            if(DEBUG_STAT) echo "get sql by type A";
            create_sql_type_repeat($sql);
            break;

        case TYPE_O:
            if(DEBUG_STAT) echo "get sql by type 0";
            create_sql_type_first($sql);
            break;

        default:                
            trigger_error(__FUNCTION__.": Parameter type in params has wrong value", E_USER_WARNING);
            return false;
    }

    sql_append_common_params($sql, $params['currency'], $site_id, $params['type'], $period['start'], $period['end']);

    if (isset($params['payment']) && !empty($params['payment']) ) {
        $sql->append(" AND o.type_payment = :s:type_payment: ", array('type_payment' => $params['payment']));
    }

    $sql->append(" GROUP BY period ");

    if(DEBUG_STAT) pr($sql->show());

    $res = $dbc->GetAll(array("balancing_mode"=>'secondary_slave'), $sql);

    return $res;
}

function create_sql_type_first(SQL $sql) {

    if(!($sql instanceof SQL)) {
        exit('sql in create_sql_type_first is not instanceof SQL');
        trigger_error(__FUNCTION__.": Result of get_order_data_by_period() is empty", E_USER_WARNING);
        return false;
    }

    $sql->create("
        SELECT
            date(tdate) as period,  count(*) as cnt  
        FROM
            orders o            
        WHERE
            o.apr_code IS NOT NULL
            AND o.apr_code <> ''               
    ");

    return true;        
}

function create_sql_type_repeat(SQL $sql) {

    if(!($sql instanceof SQL)) {
        exit('sql in create_sql_type_repeat is not instanceof SQL');
        trigger_error(__FUNCTION__.": Result of get_order_data_by_period() is empty", E_USER_WARNING);
        return false;
    }

    $sql->create("
        SELECT
            date(tdate) as period,  count(*) as cnt  
        FROM
            orders o
        INNER JOIN
            repeat_scheduled rs
        ON
            (o.user_id = rs.pid AND rs.placed_on = date(o.tdate) AND o.id = rs.order_id_created)  
        WHERE
            o.apr_code IS NOT NULL
            AND o.apr_code <> ''              
            AND rs.is_retrying = 0  
            AND rs.status = 'FAILED'
    ");

    return true;
}

function sql_append_common_params(SQL $sql, $currency, $site_id, $type, $period_start, $period_end) {

    $arg_list = func_get_args();
    for ($i = 1; $i < count($arg_list); $i++) {                        
        if(empty($arg_list[$i])) {                
            trigger_error(__FUNCTION__.": We don't have {$arg_list[$i]}", E_USER_WARNING);                
        } 
    }                

    $sql->append(" AND o.currency = :s:currency: ", array('currency' => $currency.":1"));
    $sql->append(" AND o.siteID = :i:siteID: ", array('siteID' => $site_id));
    $sql->append(" AND o.type = :s:type: ", array('type' => $type));
    $sql->append(" AND DATE(o.tdate) BETWEEN :s:start: AND :s:end:  ",
        array('start' => $period_start, 'end'=> $period_end)
    );

    return true;
}

function get_order_message(SQL $sql, DatingDB $dbc, array $params, $site_id, array $period_time) {

    if(DEBUG_STAT)
        echo "<p>Call get_order_message</p>";        

    $type         = $params['type'];
    $payment_type = $params['payment'];
    $currency     = $params['currency'];

    switch($type) {
    case TYPE_A:
        if ($payment_type=='fghue')
        {
            $sql->create("
                SELECT
                    date(tdate) as period,  
                    sum(if(o.apr_code='CHARGED',1,0)) as 'CHARGED',  
                    sum(if(o.apr_code='REFUNDED',1,0)) as 'REFUNDED',  
                    SUBSTRING(note,LOCATE('<resulttext>',note)+LENGTH('<resulttext>'),LOCATE('</resulttext>',note)-LOCATE('<resulttext>',note)-LENGTH('</resulttext>')+1) as msg,  
                    count(*) as cnt  
                FROM
                    orders o
                INNER JOIN
                    repeat_scheduled rs ON (o.user_id = rs.pid AND rs.placed_on = date(o.tdate) AND o.id = rs.order_id_created)  
                WHERE
                    o.type_payment = 'fghue'  

            ");
        }
        elseif ($payment_type=='sdgsdg')
        {
            $sql->create("
                SELECT
                    date(tdate) as period,  
                    sum(if(o.apr_code='CHARGED',1,0)) as 'CHARGED',  
                    sum(if(o.apr_code='REFUNDED',1,0)) as 'REFUNDED',  
                    SUBSTRING(note,LOCATE('<text>',note)+LENGTH('<text>'),LOCATE('</text>',note)-LOCATE('<text>',note)-LENGTH('</text>')+1) as msg,  
                    count(*) as cnt  
                FROM
                    orders o
                INNER JOIN repeat_scheduled rs ON (o.user_id = rs.pid AND rs.placed_on = date(o.tdate) AND o.id = rs.order_id_created)  
                WHERE               
                    o.type_payment = 'sdgsdg'            
            ");

        }
        elseif ($payment_type=='dfg')
        {
            $sql->create("
                SELECT
                    date(tdate) as period,  
                    sum(if(o.apr_code='CHARGED',1,0)) as 'CHARGED',  
                    sum(if(o.apr_code='REFUNDED',1,0)) as 'REFUNDED',  
                    SUBSTRING(note,LOCATE('[transactionError]',note)+LENGTH('[transactionError]'),LOCATE('[transactionStatus]',note)-LOCATE('[transactionError]',note)-LENGTH('[transactionStatus]')+1) as msg,  
                    count(*) as cnt  
                FROM
                    orders o
                INNER JOIN repeat_scheduled rs ON (o.user_id = rs.pid AND rs.placed_on = date(o.tdate) AND o.id = rs.order_id_created)  
                WHERE
                  o.type_payment = 'dfg'
            ");

        }
        elseif ($payment_type=='dkijhfw')
        {
            $sql->create("
                SELECT
                    date(tdate) as period,  
                    sum(if(o.apr_code='CHARGED',1,0)) as 'CHARGED',  
                    sum(if(o.apr_code='REFUNDED',1,0)) as 'REFUNDED',  
                    LEFT(SUBSTRING(note,LOCATE(:s:t1:,note)+LENGTH(:s:t1:),LOCATE(:s:t2:,note)-LOCATE(:s:t1:,note)-LENGTH(:s:t2:)-1), 40) as msg,  
                    count(*) as cnt  
                FROM
                    orders o
                    INNER JOIN repeat_scheduled rs ON (o.user_id = rs.pid AND rs.placed_on = date(o.tdate) AND o.id = rs.order_id_created)  
                WHERE
                   o.type_payment = 'dkijhfw'",
                   array('t1' => 'REASON :: ', 't2' => 'ERRCODE ::')
            );

        }
        else
        {
            die('Payment system does not supported by this report');
        }

        sql_append_common_params($sql, $currency, $site_id, $type, $period_time['start'], $period_time['end']);

        $sql->append(" AND rs.is_retrying = :i:is_retrying: ", array('is_retrying' => 0));            
        $sql->append(" GROUP BY period, msg ");

        break;

    case TYPE_O:
        if ($payment_type=='fghue')
        {        

            $sql->create("
                SELECT
                    date(tdate) as period,  
                    sum(if(o.apr_code='CHARGED',1,0)) as 'CHARGED',  
                    sum(if(o.apr_code='REFUNDED',1,0)) as 'REFUNDED',  
                    SUBSTRING(note,LOCATE('<resulttext>',note)+LENGTH('<resulttext>'),LOCATE('</resulttext>',note)-LOCATE('<resulttext>',note)-LENGTH('</resulttext>')+1) as msg,  
                    count(*) as cnt  
                FROM
                    orders o                    
                WHERE
                    o.type_payment = 'fghue'  

            ");
        }
        elseif ($payment_type=='sdgsdg')
        {
            $sql->create("
                SELECT
                    date(tdate) as period,  
                    sum(if(o.apr_code='CHARGED',1,0)) as 'CHARGED',  
                    sum(if(o.apr_code='REFUNDED',1,0)) as 'REFUNDED',  
                    SUBSTRING(note,LOCATE('<text>',note)+LENGTH('<text>'),LOCATE('</text>',note)-LOCATE('<text>',note)-LENGTH('</text>')+1) as msg,  
                    count(*) as cnt  
                FROM
                    orders o                    
                WHERE               
                    o.type_payment = 'sdgsdg'            
            ");

        }
        elseif ($payment_type=='sdgasdg')
        {
            $sql->create("
                SELECT
                    date(tdate) as period,  
                    sum(if(o.apr_code='CHARGED',1,0)) as 'CHARGED',  
                    sum(if(o.apr_code='REFUNDED',1,0)) as 'REFUNDED',  
                    SUBSTRING(note,LOCATE('[transactionError]',note)+LENGTH('[transactionError]'),LOCATE('[transactionStatus]',note)-LOCATE('[transactionError]',note)-LENGTH('[transactionStatus]')+1) as msg,  
                    count(*) as cnt  
                FROM
                    orders o                    
                WHERE
                  o.type_payment = 'sdgasdg'
            ");

        }
        elseif ($payment_type=='dkijhfw')
        {
            $sql->create("
                SELECT
                    date(tdate) as period,  
                    sum(if(o.apr_code='CHARGED',1,0)) as 'CHARGED',  
                    sum(if(o.apr_code='REFUNDED',1,0)) as 'REFUNDED',  
                    LEFT(SUBSTRING(note,LOCATE(:s:t1:,note)+LENGTH(:s:t1:),LOCATE(:s:t2:,note)-LOCATE(:s:t1:,note)-LENGTH(:s:t2:)-1), 40) as msg,  
                    count(*) as cnt  
                FROM
                    orders o                        
                WHERE
                   o.type_payment = 'dkijhfw'",
                   array('t1' => 'REASON :: ', 't2' => 'ERRCODE ::')
            );

        }
        else
        {
            die('Payment system does not supported by this report');
        }

        sql_append_common_params($sql, $currency, $site_id, $type, $period_time['start'], $period_time['end']);

        $sql->append(" GROUP BY period, msg ");

        break;

        default:
            exit('Wrong payment type');

    }

    if(DEBUG_STAT) {
        pr($sql->show());
    }

    $res = $dbc->GetAll(array("balancing_mode"=>'secondary_slave', 'cache'=>true, 'lifetime'=>3600), $sql);

    return $res;
}

?>

mseancole · Answer 1 · 2012-04-19 21:04:06Z

I'll be honest, I just skimmed most of this. As GordonM said, it really should be commented, especially for such a large procedural file. If you are still watching this thread, here are a few things to look into.

WARNING Large wall of text ahead! WARNING

Code Separation

This file is LARGE. Not in size, but in function. You should really think about separating your code into logical parts, before, after, and during the coding process. Separating the HTML from the PHP isn't just good practice, its easier to read and will allow others with less programming knowledge, such as designers, to work on the project with you. Some may argue that this is just a preference, but I'd argue its almost a necessity. Extending this advice: This also works for separating parts of code. If you are doing a form, create a new PHP file for form processing. Don't lump all your code into one file just because they do similar things. Even if you only separate your functions from your procedural code you will notice immediate results.

Using Functions

Using functions, properly, goes hand-in-hand with code separation, but deserves its own section. You have the right idea, but you can do so much more with your functions. You don't have to do everything within the same function, just as you don't have to do everything within the same file. Think of it as a small step on the way to code separation. Break everything down into reusable parts and make functions out of them.

Take a look at your get_order_message function. You use very similar code blocks throughout that entire function. I didn't read every single line, but from what I gather there are only 2 real changes between each $sql->create() calls. Create another function to do those SQL calls and pass the right variables and SQL string parts. After you've done that you'll notice a hefty chunk has been taken out of your code. Take a look at your get_order_message after adding said function. There are still some things that still need to be changed, but this will get you started.

function sqlCreate($substring, $type) {
    $sql->create("
        SELECT
            date(tdate) as period,  
            sum(if(o.apr_code='CHARGED',1,0)) as 'CHARGED',  
            sum(if(o.apr_code='REFUNDED',1,0)) as 'REFUNDED',  
            SUBSTRING(note, $substring +1) as msg,
            count(*) as cnt  
            FROM
                orders o
            INNER JOIN
                repeat_scheduled rs ON (
                    o.user_id = rs.pid AND rs.placed_on = date(o.tdate) AND o.id = rs.order_id_created
                )  
            WHERE
                o.type_payment = $type  
    ");

}

function get_order_message() {
    sqlCreate(
        "LOCATE('<resulttext>',note)+LENGTH('<resulttext>'),LOCATE('</resulttext>',note)-LOCATE('<resulttext>',note)-LENGTH('</resulttext>')",
        $payment_type
    );
}

Go wild with it, get your code as reusable as possible. While not always practical it will get you in the right mindset. Reusable is better. Not just for OOP, but for procedural programming as well. The less you have to type, the better and the less you have to type the less you will have to update and maintain.

Using Loops

Don't be afraid of loops either (for, while, foreach, etc...). You have multiple instances where you could replace a large chunk of code with just a few lines and be done. Not to mention this makes your code extendable. This is just an example:

/*This...
$payment_type = ((isset($_GET['payment'])  && !empty($_GET['payment'])))? $_GET['payment'] : 'fghue';
$currency     = ((isset($_GET['currency']) && !empty($_GET['currency'])))? $_GET['currency'] : 'GBP';
$type         = ((isset($_GET['type'])
can be converted to this with just a few tweaks...*/
$paymentInfo = array(
    'payment' => '',
    'currentcy' => '',
    'type' => ''
);
foreach(array_keys($paymentInfo) as $key) { $paymentInfo[$key] = $_GET[$key]; }

Ternary Operators

Ternary operators are good so long as they are understandable. Once they start getting too bulky they lose their legibility and usefulness. If you wish to use ternary operators, you should start condensing where you can. That is, after all, the whole point of using them in the first place. You can start with those $_GET calls.

function filter($var, $default = FALSE) {
    return (isset($_GET[$var]) && ! empty($_GET[$var]) ? $_GET[$var] : $default);
}

$payment_type = filter('payment', 'fghue');

See how I slipped a function in there? Just abstracting it that much made that ternary operation so much easier to read.

But you can do more. If your PHP version is 5.2 or greater you can just use the filter_input() method. It is a PHP function that does exactly what you are already doing, plus more. If there is a value to return it will return it based on the filter flags you provide, otherwise it will return FALSE. Of course, just using filter_input by itself wont be much shorter than what you already have, but combine that with the function I gave you above and:

function filter($var, $flags = null) {
    $method = array(
        'get' => INPUT_GET,
        'post' => INPUT_POST,
    );
    $filter = array(
        'string' => FILTER_SANITIZE_STRING | FILTER_SANITIZE_STRIPPED,
        'int' => FILTER_VALIDATE_INT,
    );

    if( ! $flags) {
        $filterType = $filter['string'];
        $methodType = $method['get'];
    } else {
        extract($flags);//sets $filterType and $methodType
        $filterType = $filter[$filterType];
        $methodType = $method[$methodType];
    }
    return filter_input($methodType, $var, $filterType);
}

$flags = array(
    'filterType' => 'string',
    'methodType' => 'get'
);
$payment_type = filter('payment');//with defaults
$payment_type = filter('payment', $flags);//with custom settings

//If you want to check if it was set to give it a default value...
$payment_type = $payment_type ?: 'fghue';//PHP 5.3 and later can use ternary shorthand "?:"
$payment_type = $payment_type ? $payment_type : 'fghue';//Older cannot

Magic Values/Variables

All those random strings fghue are what I mean by "magic". They came out of no where and mean absolutely nothing to a majority of us. And in a year when you are looking back on this project it may not mean anything to you either. Make life simpler by telling us what those strings are. Its called self-documenting code and makes reading your code easier. While this may make your code larger, it will be for the right reasons. No one is going to complain about you making your code more legible, in fact many programmers might just break into sobs of joy.

define('descriptive_name', 'fghue');//if the name isn't enough add a comment
//etc...
$payment_type = $payment_type ?: descriptive_name;

HTML Output

Don't echo HTML output unless absolutely necessary. In case you are confused, its only necessary when you need to use PHP variables in it, and not always then. This is another one of those "Separation of Code" things. You retain tag highlighting and autocompletion features as well as other benefits. If you MUST echo HTML output try to use heredoc as it is still more readable than other alternatives.

    echo <<<HTML
<a href="$href">$link</a>
<$tag>asdf</$tag>
HTML;//notice this closure element is not indented

Much better to just escape from PHP and then enter back in to echo a single variable if you can.

... ?>
<a href="<?php echo $href; ?>"><?php echo $link; ?></a>
<?php ...

Discalimer: Convert it to a Class

Don't! No matter what anyone else says, don't do it. Don't worry, I'm not against OOP and I'm not trying to steer you wrong. I'm trying to save you a lot of heartache and frustration. You obviously aren't ready for OOP quite yet. Don't take that the wrong way, I'm still struggling with it myself. It takes quite a bit of practice, and before you can get started you have to know your basics. Take it slow. Don't immediately jump into converting all your PHP files into classes because someone told you it was better. You'll only give yourself a headache and discourage yourself. I hated seeing answers like that when browsing forums and I'll never give that advice to anyone because of it. Don't get me wrong, OOP is definately a life saver and something you should eventually look into, but you should be comfortable and proficient with procedural programming first. The only reason I mentioned this at all is because of how likely it is that you WILL receive this "advice" from someone at some point. Just go on ignoring them until you are ready to tackle it on your own.

Conclusion

I hope these suggestions helped and that I didn't go off on too much of a tangeant or lose you somewhere. I kind of got carried away. Ask questions, not just here, but on other forums as well, thats how we learn :)

asked	1 year ago
viewed	179 times
active	1 year ago

Please review my php code (It's report from order table)

1 Answer

Your Answer

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

Please review my php code (It's report from order table)

1 Answer

Your Answer

Sign up or log in

Post as a guest

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

Related