1
\$\begingroup\$

I wrote a PHP function that generates JSON data to populate a line graph. Through jQuery, my code uses a GET request to the PHP script to fetch the JSON data. An example of the JSON data is as follows:

{"labels":[{"x":"2013-10-30","a":"1","b":"8"},{"x":"2013-10-31","a":"2","b":"14"},{"x":"2013-11-01","a":"5","b":"12"}]}

The data is used to generate a time graph showing two values per date:

x: date a: value1 b: value2

I'm still a beginner programmer and want to have some peer review on the code I wrote. Please give me some feedback on how I can improve it, and fix any errors or inefficient methods if present.

Function

function generateGraphData($start, $end, $format, $api, $server, $pdo)
{
    //Fetch unique entries between the given dates
    $query = "SELECT DISTINCT DATE(traffic_date) FROM traffic WHERE traffic_date BETWEEN ? AND ? AND traffic_api = ? AND traffic_server = ? ORDER BY traffic_date DESC";
    $dbc = $pdo->prepare($query);  
    $dbc->execute(array($start, $end, $api, $server));
    $results = $dbc->fetchAll(PDO::FETCH_ASSOC);

    //Count unique and total views for each result
    foreach ($results as $key => $value)
    {
        $date = "%".$value['DATE(traffic_date)']."%";

        $dbc = $pdo->prepare("SELECT count(*) FROM traffic WHERE traffic_date LIKE ? AND traffic_api = ? AND traffic_server = ?");
        $dbc->execute(array($date, $api, $server));
        $total = $dbc->fetchColumn();
        $dbc = null;

        $dbc = $pdo->prepare("SELECT count(*) FROM traffic WHERE traffic_date LIKE ? AND traffic_api = ? AND traffic_server = ? GROUP BY traffic_ip");
        $dbc->execute(array($date, $api, $server));
        $unique = $dbc->fetchColumn();
        $dbc = null;

        $data[$key]["x"] = $value['DATE(traffic_date)'];        
        $data[$key]["a"] = $unique;
        $data[$key]["b"] = $total;
    }

    //Create filler dates
    $fillerDates = new DatePeriod(new DateTime($start), new DateInterval('P1D'), new DateTime($end));

    foreach($fillerDates as $date)
    {
        $filler_date_array[] = array('x' => $date->format("Y-m-d"), 'a' => '0', 'b' => '0');    
    }       

    $data = array_merge($data, $filler_date_array); 

    //Remove filler dates if real date exists
    $filter_unique_array = array();
    $keysArray = array();

    foreach ($data as $innerArray)
    { 
        if (!in_array($innerArray["x"], $keysArray))
        { 
            $keysArray[] = $innerArray["x"]; 
            $filter_unique_array[] = $innerArray;
        }
    }

    //Sort by date
    array_sort_by_date($filter_unique_array, "date");

    //Format JSON
    $json = json_encode($filter_unique_array);
    $jsonlabel = '{"labels":'.$json.'}';
    echo $jsonlabel;    
}
\$\endgroup\$
4
  • \$\begingroup\$ you are executing three different sql. Here you should execute a query only once and fetch all records. It will improve your code execution time. When you have all records, you could just iterate through the result and compute "total" and "unique" values. \$\endgroup\$
    – Kinjal
    Commented Nov 5, 2013 at 4:54
  • \$\begingroup\$ I was thinking this too, but how would I approach this? I just started using PDO, and "beginner/intermediate" SQL queries is still hard for me. \$\endgroup\$
    – Bob Jansen
    Commented Nov 5, 2013 at 14:58
  • \$\begingroup\$ Would this be be correct? SELECT DISTINCT DATE(traffic_date), count(traffic_ip), count(distinct traffic_ip) FROM traffic WHERE traffic_date BETWEEN ? AND ? AND traffic_api = ? AND traffic_server = ? GROUP BY DATE(traffic_date) \$\endgroup\$
    – Bob Jansen
    Commented Nov 5, 2013 at 16:31
  • \$\begingroup\$ you could try this select traffic_date, count(1) from traffic where traffic_date BETWEEN ? AND ? and traffic_api = ? and traffic_server = ? group by traffic_date, traffic_ip; this will give all result and required counts, then you can iterate the result set in php \$\endgroup\$
    – Kinjal
    Commented Nov 5, 2013 at 16:45

1 Answer 1

1
\$\begingroup\$

The first problem is what @Kinjal identified in your comment. You should only run your query once (in this particular case. You can do this because your querys basically return the same result).

So you run your query and load it into a table, and then you can iterate and work with that table. This will be much faster than running your query 3 times. Even so because 2 of your queries runs in a loop.

The second problem are your JSON members. using "a" and "b" as member names are a bad idea - name these what they should represent. I'd suggest substituting them for "unique" and "total". So your final JSON should looke like

{"date": "yyyy-mm-dd", "unique": 0, "total": 0 }

\$\endgroup\$
1
  • \$\begingroup\$ Thanks for the feedback. As I commented above to @Kinjal, I'm wondering how I should approach this the correct way. SQL queries are still quite new too me. \$\endgroup\$
    – Bob Jansen
    Commented Nov 5, 2013 at 15:01

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