5
\$\begingroup\$

I have the following function that works fine. Is there a better way to do it and will this be efficient if I have 100+ rows returned from each table?

The database

Tables

  • wcx_articles
  • wcx_videos
  • wcx_apps
  • wcx_links

The corresponding _active column for each table

  • article_active
  • video_active
  • app_active
  • link_active

_active is a switch 1 or 0

This is my current function

public function findInactiveItems() {

    $query = "SELECT * FROM wcx_articles WHERE article_active = '0'
    UNION
    SELECT * FROM wcx_videos WHERE video_active = '0'
    UNION
    SELECT * FROM wcx_apps WHERE app_active = '0'
    UNION
    SELECT * FROM wcx_links WHERE link_active = '0'
    ";
        if( $this->num_rows( $query ) > 0 ) {    
            $count = $this->num_rows($query);
            return $count;
        }
    }

 public function num_rows( $query ) {
    $query = $this->link->query( $query );
    if( mysqli_error( $this->link ) ) {
        $this->log_db_errors( mysqli_error( $this->link ), $query, 'Fatal' );
        return mysqli_error( $this->link );
    }
    else {
        return mysqli_num_rows( $query );
    }
    mysqli_free_result( $query );
}
\$\endgroup\$
  • 1
    \$\begingroup\$ Updated my question with working code \$\endgroup\$ – CodeX Jun 26 '14 at 13:15
  • \$\begingroup\$ Follow-up question \$\endgroup\$ – 200_success Jun 26 '14 at 23:50
5
\$\begingroup\$

SQL

If you want a count, ask for a count. Don't ask the server to send all the data for all the rows, just to have the client count the rows. (A few hundred rows is a trivial amount of data for a database to handle, but still, you should follow good practices.)

SELECT SUM(inactive_count) AS inactive_count
    FROM (
        SELECT COUNT(article_active) AS inactive_count
            FROM wcx_articles WHERE article_active = '0'
        UNION ALL
        SELECT COUNT(video_active)
            FROM wcx_videos WHERE video_active = '0'
        UNION ALL
        SELECT COUNT(app_active)
            FROM wcx_apps WHERE app_active = '0'
        UNION ALL
        SELECT COUNT(link_active)
            FROM wcx_links WHERE link_active = '0'
    ) AS wcs_objects;

Schema

If in fact all four of your tables are union-compatible with each other, then you should strongly consider merging them into one table with a wcx_type attribute that declares what type of object that row represents.

Are your …_active columns really strings? Those columns should be of the BOOLEAN (i.e., TINYINT(1)) type. The query should be adjusted accordingly: use …_active = 0 instead of …_active = '0'.

PHP

You reused the $query parameter to store the result of the query. Don't be stingy like that — it's confusing. Use a new variable to store the result.

You have a leak: mysqli_free_result( $query ); never gets called, because that statement is unreachable.

You are mixing the object-oriented and procedural styles of . You should either use the object-oriented style consistently…

public function num_rows($query) {
    if (!($result = $this->link->query($query))) {
        $this->log_db_errors($this->link->error, $result, 'Fatal');
        return $this->link->error;
    }
    $num_rows = $result->num_rows;
    $result->free();
    return $num_rows;
}

or use the procedural style consistently…

public function num_rows($query) {
    if (!($result = mysqli_query($this->link, $query))) {
        $this->log_db_errors(mysqli_error($this->link), $result, 'Fatal');
        return mysqli_error($this->link);
    }
    $num_rows = mysqli_num_rows($result);
    mysqli_free_result($result);
    return $num_rows;
}
\$\endgroup\$
  • \$\begingroup\$ Maybe I should have mentioned: when using the COUNT(*) query, you no longer want mysqli_num_rows() or $result->num_rows. Rather, you should ask for the inactive_count of the first row of the result. \$\endgroup\$ – 200_success Jun 26 '14 at 16:29
  • \$\begingroup\$ Hmm Undefined index: inactive_count, should i use a straight query on this and return a result \$\endgroup\$ – CodeX Jun 26 '14 at 18:49
  • \$\begingroup\$ Works for me: SQL Fiddle \$\endgroup\$ – 200_success Jun 26 '14 at 18:52
  • \$\begingroup\$ Yeah it works for me in phpmyadmin SQL, my php must be wrong, im trying to get the $row['inactive_count'] from an assoc array \$\endgroup\$ – CodeX Jun 26 '14 at 18:56
  • \$\begingroup\$ This works but is it right? $row = $this->get_results($query); $sum = $row[0]; foreach($sum as $sum) {echo $sum;} \$\endgroup\$ – CodeX Jun 26 '14 at 19:11

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.