2
\$\begingroup\$

I have some PHP code I'm working on which contains 2 queries and I need to set extra conditions for 2 out of 3 view options and one less query for 1 out of 3 view options.

Basically, I'm rendering a history list of shows with three options ALL | PRO | UPCOMING. The ALL option should include both queries and have no extra conditions. The PRO option should have an extra condition with both queries. The UPCOMING option should have an extra condition but only one query.

I've never built an if else statement with queries before, but I'm sure there is an easier way to make the code shorter.

This PHP is also within an API that get pulled to the front end. Is there a way to render buttons on the front end to change the query from ALL to PRO or UPCOMING?

 function get_shows($playid) {

    // There are two tables, so we will need to merge the results
    $results = array();

    $count  = ($this->input->get('count')) ? $this->input->get('count') : $this->max_results;
    $offset = ($this->input->get('offset')) ? $this->input->get('offset') : 0;
    $view = 'all';

    // ALL
    if ($view == 'all') {
        $extra_condition = ""; //If ALL no extra condition needs to be set

    $query = $this->db->query("SELECT distinct customer.customerid as customerid, organization, vcity, vstate, vcountry, firstdate, lastdate 
                               FROM customer, orders, venue 
                               WHERE orders.ordersid=venue.ordersid 
                               AND orders.playid=? AND orders.active=1 
                               AND customer.customerid=orders.customerid 
                               AND shipped != 0 
                               AND defunct != 1 
                               AND orders.lastdate != 0 
                               AND (ordertype != 'misc' 
                               AND ordertype != 'reading') 
                               AND hidepic!=1 
                               AND hideprod!=1 
                               AND num_perf>0 
                               ORDER BY firstdate desc 
                               LIMIT $offset,$count", $playid);
    foreach ($query->result() as $row)
    {
        $results[] = array 
        (
    'id' => $row->customerid,
    'src' => 'customerid',
            'organization' => $row->organization,
            'city' => $row->vcity,
            'state' => $row->vstate,
            'country' => $row->vcountry,
            'firstdate' => call_user_func($fmt_date, $row->firstdate),
            'lastdate' => call_user_func($fmt_date, $row->lastdate)
        );
    }

    // Legacy data from oldprods
    $query = $this->db->query("SELECT oldprodsid, organization, city, state, country, firstdate, lastdate  
                               FROM oldprods where playid=? 
                               ORDER BY firstdate desc 
                               LIMIT $offset,$count",$playid);
    foreach ($query->result() as $row)
    {
        $results[] = array 
        (
    'id' => $row->oldprodsid,
    'src' => 'oldprods',
            'organization' => $row->organization,
            'city' => $row->city,
            'state' => $row->state,
            'country' => $row->country,
            'firstdate' => call_user_func($fmt_date, $row->firstdate),
            'lastdate' => call_user_func($fmt_date, $row->lastdate)
        );
    }

    // IF UPCOMING
    }else if ($view == 'upcoming') {
    $extra_condition = "AND lastdate >= DATE_SUB(CURRENT_DATE,INTERVAL 0 DAY)"; //This line of code designates this as UPCOMING but omit the Legacy Query
    $query = $this->db->query("SELECT distinct customer.customerid as customerid, organization, vcity, vstate, vcountry, firstdate, lastdate 
                               FROM customer, orders, venue 
                               WHERE orders.ordersid=venue.ordersid 
                               AND orders.playid=? AND orders.active=1 
                               AND customer.customerid=orders.customerid 
                               AND shipped != 0 
                               AND defunct != 1 
                               AND orders.lastdate != 0 
                               AND (ordertype != 'misc' 
                               AND ordertype != 'reading') 
                               AND hidepic!=1 
                               AND hideprod!=1 
                               AND num_perf>0 
                               $extra_condition 
                               ORDER BY firstdate desc 
                               LIMIT $offset,$count", $playid);
    foreach ($query->result() as $row)
    {
        $results[] = array 
        (
    'id' => $row->customerid,
    'src' => 'customerid',
            'organization' => $row->organization,
            'city' => $row->vcity,
            'state' => $row->vstate,
            'country' => $row->vcountry,
            'firstdate' => call_user_func($fmt_date, $row->firstdate),
            'lastdate' => call_user_func($fmt_date, $row->lastdate)
        );
    }

    // IF PRO
    }else if ($view == 'pro') {
    $extra_condition = "AND (ordertype = 'pro')"; //This line of code designates this as PRO along with the Legacy Query
    $query = $this->db->query("SELECT distinct customer.customerid as customerid, organization, vcity, vstate, vcountry, firstdate, lastdate 
                               FROM customer, orders, venue 
                               WHERE orders.ordersid=venue.ordersid 
                               AND orders.playid=? AND orders.active=1 
                               AND customer.customerid=orders.customerid 
                               AND shipped != 0 
                               AND defunct != 1 
                               AND orders.lastdate != 0 
                               AND (ordertype != 'misc' 
                               AND ordertype != 'reading') 
                               AND hidepic!=1 
                               AND hideprod!=1 
                               AND num_perf>0 
                               $extra_condition 
                               ORDER BY firstdate desc 
                               LIMIT $offset,$count", $playid);
    foreach ($query->result() as $row)
    {
        $results[] = array 
        (
    'id' => $row->customerid,
    'src' => 'customerid',
            'organization' => $row->organization,
            'city' => $row->vcity,
            'state' => $row->vstate,
            'country' => $row->vcountry,
            'firstdate' => call_user_func($fmt_date, $row->firstdate),
            'lastdate' => call_user_func($fmt_date, $row->lastdate)
        );
    }

    // Legacy data from oldprods
    $query = $this->db->query("SELECT oldprodsid, organization, city, state, country, firstdate, lastdate  
                               FROM oldprods where playid=? 
                               ORDER BY firstdate desc 
                               LIMIT $offset,$count",$playid);
    foreach ($query->result() as $row)
    {
        $results[] = array 
        (
    'id' => $row->oldprodsid,
    'src' => 'oldprods',
            'organization' => $row->organization,
            'city' => $row->city,
            'state' => $row->state,
            'country' => $row->country,
            'firstdate' => call_user_func($fmt_date, $row->firstdate),
            'lastdate' => call_user_func($fmt_date, $row->lastdate)
        );
    }

    }
    return array_slice($results, 0, $count);
}
\$\endgroup\$
4
  • \$\begingroup\$ Welcome to Code Review! Are you asking code to be written ? Here at Code Review, we are reviewing working and complete code. If you need help to code something this is not the good site. \$\endgroup\$ – Marc-Andre Feb 28 '14 at 14:34
  • \$\begingroup\$ I had working code but it's very long since the only way I could get things partially working was to have multiple duplicate queries. I have posted the original code above. \$\endgroup\$ – Phillip De Vita Feb 28 '14 at 15:06
  • 1
    \$\begingroup\$ Thanks for the edit, the more important part of a review here is having a code that is working. That your code is long and not optimal is nothing to worry, Code Review is here to help you get to something better and hope to help you learn in the process. \$\endgroup\$ – Marc-Andre Feb 28 '14 at 15:21
  • \$\begingroup\$ @spencer7593 Spencer this is great and works perfectly. Reason I had set the $view='all' was to test the functionality of each view on the front end. This code works great but ideally I need to create buttons on the front end that would change the different views. If this was all part of the existing page it would be easy as I could link the page back to itself something like: <a href="prodhistory.php?playid=$playid&view=pro">PRO</a> But since this is in the API I can't do that. I'm wondering if there is a way to do it through javascript though via an onclick or onchange? \$\endgroup\$ – Phillip De Vita Mar 3 '14 at 14:02
1
\$\begingroup\$

(What's curious is that you are setting $view = 'all', and then immediately testing if it's set to 'upcoming' or 'pro', I don't see any where else that $view is being assigned a value.)

But, with that issue aside, it took me a bit to figure out that there are huge chunks of code that is replicated. (I actually copied chunks out and saved them as separate files, so I could run compares... and the blocks are IDENTICAL except for the $extra_condition, and the legacy query doesn't get run for 'upcoming', only for 'all' and 'pro'.

I've minimally refactored your code to remove the replicated code. There's probably other changes that can be made, but in terms of a reader figuring out what this code does, removing the redundant code is the first priority.

It would be MUCH easier on the reader if the identical query blocks weren't repeated. In the case of the 'all' view, we can think of $extra_condition as really just an empty string. (The way it's coded below, you'll still get the line break and a bunch of spaces; I might format the query little differently, and include the additions to the WHERE clause a little differently, but I didn't want to change anything that didn't need to be changed.)

Let's break the code into three sections:

  • setting $extra_condition
  • running the query block
  • running the legacy query block

To set $extra_condition, we test whether $view is set to 'all', 'upcoming', or 'pro', and assign the appropriate value. That makes it much easier to see what's going on, when it's set in one place, rather than being spread so far apart in the code.

Then, we'll run the query block. (We don't need to repeat that same code block three times; the code is identical in each case. We just need to check (again) whether $view is set to the one of the three values.

Next, there's a legacy query block that needs to run for two of the $view values. We have this block only once, rather than having two copies of the same code.

The only real difference (and it's not really that different at all) in what gets executed is that the SQL text (query string) for the "all" case is getting an extra empty line. That's because we're including an empty $extra_condition. That extra line is just because of the way that it's getting incorporated into the SQL text. (We could just as easily include a dummy condition like ' AND 1=1 '.)

I wasn't able to post an answer on StackOverflow yesterday because your question got closed before I could post an answer.

<?php
function get_show_($playid)
{

    // There are two tables, so we will need to merge the results for ALL and PRO
    $results = array();

    $count  = ($this->input->get('count')) ? $this->input->get('count') : $this->max_results;
    $offset = ($this->input->get('offset')) ? $this->input->get('offset') : 0;
    $view = 'all';

// SET ADDITIONS TO WHERE CLAUSE FOR QUERY BLOCK

    if ($view == 'all') {
        $extra_condition = ""; //no additions to the WHERE clause
    }else if ($view == 'upcoming') {
        $extra_condition = "AND lastdate >= DATE_SUB(CURRENT_DATE,INTERVAL 0 DAY)"; //This line of code designates this as UPCOMING but omit the Legacy Query
    }else if ($view == 'pro') {
        $extra_condition = "AND (ordertype = 'pro')"; //This line of code designates this as PRO along with the Legacy Query
    }


// RUN QUERY BLOCK

    if (($view == 'all') OR ($view == 'upcoming') OR ($view == 'pro')) {
        $query = $this->db->query("SELECT distinct customer.customerid as customerid, organization, vcity, vstate, vcountry, firstdate, lastdate
                                   FROM customer, orders, venue
                                   WHERE orders.ordersid=venue.ordersid
                                   AND orders.playid=? AND orders.active=1
                                   AND customer.customerid=orders.customerid
                                   AND shipped != 0
                                   AND defunct != 1
                                   AND orders.lastdate != 0
                                   AND (ordertype != 'misc'
                                   AND ordertype != 'reading')
                                   AND hidepic!=1
                                   AND hideprod!=1
                                   AND num_perf>0
                                   $extra_condition
                                   ORDER BY firstdate desc
                                   LIMIT $offset,$count", $playid);
        foreach ($query->result() as $row)
            {
                $results[] = array
                    (
                     'id' => $row->customerid,
                     'src' => 'customerid',
                     'organization' => $row->organization,
                     'city' => $row->vcity,
                     'state' => $row->vstate,
                     'country' => $row->vcountry,
                     'firstdate' => call_user_func($fmt_date, $row->firstdate),
                     'lastdate' => call_user_func($fmt_date, $row->lastdate)
                     );
            }
    }

// RUN LEGACY QUERY BLOCK

    if ( ($view == 'all') OR ($view == 'pro') ) {

        // Legacy data from oldprods
        $query = $this->db->query("SELECT oldprodsid, organization, city, state, country, firstdate, lastdate
                                   FROM oldprods where playid=?
                                   ORDER BY firstdate desc
                                   LIMIT $offset,$count",$playid);
        foreach ($query->result() as $row)
            {
                $results[] = array
                    (
                     'id' => $row->oldprodsid,
                     'src' => 'oldprods',
                     'organization' => $row->organization,
                     'city' => $row->city,
                     'state' => $row->state,
                     'country' => $row->country,
                     'firstdate' => call_user_func($fmt_date, $row->firstdate),
                     'lastdate' => call_user_func($fmt_date, $row->lastdate)
                     );
            }

    }
    return array_slice($results, 0, $count);
}

That's just a lot less code for a reader to wade through. And, if requirements change, and we need to a add a column to the queries, we only have two queries to change, rather than five.

There's a few other minor formatting changes I would make, but for this go around, I changed as few lines as possible. I removed a lot of redundant code, and just relocated and repeated just a few easy conditional tests.

\$\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.