Procedural PHP code for showing videos on the page, sorting them in different ways. Is this good? Should it be switched to OOP?

Question

I have just finished another page of my project and everything is working just great with the following code example.

My first concern is whether you could recommend something to make my code look more beautiful or probably more secure? As I have no user input, I suppose this code can not be SQL injected? For connecting to MySQL I use latest ADOdb. I also use pagination class as you will see below (but I didn't attach it here but it's already OO based).

My second question is if I must switch, this style of codes to OO style. I don't really understand why should I use OO so far, because, I have separate file for functions, I never repeat my self (stay "DRY"), as a result, by editing on functions it will change behavior of hundreds of other files? If you have reasons for me to use OO could you please emphasize them and could you please provide the converted working version of my code in OO so I colud study on it and compare it with my code as it's something I wrote and know thoroughly?

Thank you very much in advance,
Ilia

require( 'config.php' );
require( 'function.php' );

$active   = ( $config[ 'approve' ] == '1' ) ? " AND v.active = '1'" : NULL;
$page     = ( isset( $_REQUEST[ 'page' ] ) && is_numeric( $_REQUEST[ 'page' ] ) ) ? $_REQUEST[ 'page' ] : 1;
$sort     = ( isset( $_REQUEST[ 'sort' ] ) && $_REQUEST[ 'sort' ] ) ? $_REQUEST[ 'sort' ] : NULL;
$paginate     = ( isset( $_REQUEST[ 'paginate' ] ) && is_numeric( $_REQUEST[ 'paginate' ] ) ) ? $_REQUEST[ 'paginate' ] : NULL;

switch ( $sort )
{
    case 'popular':
        $orderBy = " ORDER BY v.viewed DESC";
        break;
    case 'highest':
        $orderBy = " ORDER BY (like_count) DESC";
        break;
    case 'lowest':
        $orderBy = " ORDER BY (dislike_count) DESC";
        break;
    case 'discussed':
        $orderBy = " ORDER BY (comment_count) DESC";
        break;
    case 'featured':
        $orderBy       = " AND v.featured = 'yes' ORDER BY v.add_time DESC";
        $sqlPaginationAppend = " AND featured = 'yes'";
        break;
    default:
        $orderBy = " ORDER BY v.add_time DESC";
        break;
}

switch ( $paginate )
{
    case '12':
        $paginationValue = '12';
        break;
    case '18':
        $paginationValue = '18';
        break;
    case '24':
        $paginationValue = '24';
        break;
    case '36':
        $paginationValue = '36';
        break;
    case '48':
        $paginationValue = '48';
        break;
    case '60':
        $paginationValue = '60';
        break;
    case '96':
        $paginationValue = '96';
        break;
    default:
        $paginationValue = '12';
        break;
}
$sqlPaginationAppend  = ( isset( $sqlPaginationAppend ) ) ? $sqlPaginationAppend : NULL;
$sqlPaginationAppend .= " AND active = '1'";
$sql                  = "SELECT count(VID) AS total_videos FROM video WHERE type = 'public'" . $sqlPaginationAppend;
$ars                  = $conn->execute( $sql );
$total                = $ars->fields[ 'total_videos' ];
$pagination           = new Pagination( $paginationValue );
$limit                = $pagination->getLimit( $total );
$sql                  = 
        "SELECT  
            v.*, 
            u.username, 
            count(DISTINCT c.comment_id) AS comment_count,
            count(DISTINCT l.UID) AS like_count,
            count(DISTINCT d.UID) AS dislike_count
        FROM 
            video AS v
        LEFT JOIN 
            users AS u ON v.UID = u.UID 
        LEFT JOIN 
            comments AS c ON v.VID = c.VID
        LEFT JOIN 
            video_rate_likes AS l ON v.VID = l.VID
        LEFT JOIN 
            video_rate_dislikes AS d ON v.VID = d.VID
        WHERE 
            v.type = 'public' " . $active . "
        GROUP BY
            v.VID
        " . $orderBy . "
        LIMIT " 
          . $limit;

$rs         = $conn->execute( $sql );
$videos     = $rs->getrows();

$paginate ? $paginate .= '/' : NULL;

$pagination_url = $config['BASE_URL']. '/videos/' .$paginate. $sort. '-{#PAGE#}';
$page_link         = $pagination->getPagination( $pagination_url );
$start_num         = $pagination->getStartItem();
$end_num           = $pagination->getEndItem();
$pagination_number = $pagination->getTotalPages();

Answer 1

Beautifying Your Code

You could start by dropping some of these parenthesis. This is purely a stylistic choice, but I view it like this: The more parenthesis there are, the harder the code is to read, especially if nesting parenthesis. If the parenthesis are necessary and numerous (more than one pair per line honestly), then it is better to separate that line into multiple lines. I'd rather see three or four lines of clean code than one extra long line that is hard to decipher because of run-on parenthesis. A couple of examples of how you can remove some of these parenthesis from your code:

require 'config.php';
$active = $config[ 'aprove' ] == 1 ? " AND v.active = '1'" : NULL;

By the way, in the above example, note that I removed the quotes from around the 1. These are unnecessary. When doing a loose comparison a string value of "1" is the same as an integer value of 1. Its only when we do an absolute comparisons that this kind of thing becomes important. Also, if the type isn't important from the start, then it is easier just to drop the quotes altogether, which is usually the case with numbers. PHP is very forgiving about its type conversion, so you can easily go back and forth between an integer and a string without any consequences.

You should refrain from using the $_REQUEST super global. This array holds all of the other super globals and overwrites certain keys based on precedence. For example, if you had both a GET and a POST "page" variable then using REQUEST would return the POST version. The precedence is logical, but its better to explicitly declare from where you are expecting this value to come from in case there are issues. Additionally, no matter which super global you end up using it is always a good idea to validate and sanitize the results. You can check out PHP's filter_input() if your PHP version is >= 5.2.

$page = filter_input( INPUT_GET, 'page', FILTER_SANITIZE_NUMBER_INT );

DRY isn't just about not repeating functional code, but not repeating anything. This makes it easier to make updates should you decide to change how a certain feature is implemented. For example, that first switch is full of DRY violations. If you create a template for the $orderBy results, then modify the template variables in the switch, then you will more easily be able to update this code should you ever decide that ordering by descending value isn't what you want to do.

$and = '';
$by = 'v.add_time';

switch( $sort ) {
    case 'popular' :
        $by = 'v.viewed';
    break;
    //etc...
    case 'featured' :
        $and = " AND v.featured = 'yes'";
        $sqlPaginationAppend = " AND featured = 'yes'";
    break;
}

$orderBy = "$and ORDER BY $by DESC";

What is the point of this second switch? It is extremely redundant and has too much overhead. Just set the two variables equal to each other. If you need to ensure that $paginate defaults to 12 if its value is too large, use a greater than comparison. If you need to do so for a list of variables, do an in_array() check.

$paginationValue = $paginate;
//OR
$paginationValue = $paginate > $max ? 12 : $paginate;
//OR
$whitelist = array( 12, 18, 24, 36, 48, 60, 96 );
$paginationValue = in_array( $paginate, $whitelist ) ? $paginate : 12;

Of course, with the last of the above examples you start violating the rule about magic numbers, so these should actually be part of a config file as constants you have set, then you can use the defined() function instead of in array, but that's altogether different and is usually done when upgrading to an OOP style. Though this is still some good information and couldn't hurt to read up on.

Instead of checking to see if a variable is set to determine if it needs a default value, just set it with the default value and override that value whenever you need to. This saves a function call and makes your code a little more natural to read.

$sqlPaginationAppend  = NULL;
//etc...
    case 'featured':
        $orderBy       = " AND v.featured = 'yes' ORDER BY v.add_time DESC";
        $sqlPaginationAppend = " AND featured = 'yes'";
        break;
//etc...

Security

As for security, well MySQL is usually frowned upon now. A better way to go would be MySQLi or PDO. I think PDO is the more popular of the two, but I am unfamiliar with both, so I can't help you here.

OOP

It's always a good idea to adopt an OOP style for larger projects, but you have to weigh the benefits versus your project's scope. When starting a large project you have the advantage of not having developed anything yet, so you can decide what design pattern you want to implement and work towards that goal. However, you mentioned something about having hundreds of files already. Converting these all to OOP will be a huge task and may not be worth the time or effort. You have to decide if this is something that is feasible for you. I did it on one of my projects, and while it was boring and tedious as hell, the overall benefits for me were worth it. It is now much more easier to update and maintain my code, and I am still actively working on adding new features, so the OOP framework allows me to more easily add things without breaking the rest. This will also ensure that people who come after me will be able to more easily integrate into it. So you see, there are a great many things to take into account.

By the way, it appears that you have the misconception that just because something is in a class that it is OOP. OOP is more than just classes, it is how those classes interact. It doesn't even really have to be classes. From my understanding, you can do OOP with functions as well, but conventionally, it is more commonly accepted that a class should contain OOP code. In fact, your code, at least in its separation and purpose, already resembles OOP. The only thing this is really missing is for everything to be contained within logical functions that can be reused and communicate with eachother.

As for reworking your code into OOP for you? I'm sorry, but no. Not that I don't want to help, but that's the wrong way to go about it. My implementation may not be the right one. I would make this a Model for an MVC framework. Because that's the one I'm most familiar with and what it most resembles to me. But who's to say that this is the right one? There isn't enough code here to make a good determination. Take a look at wikipedia's article on design patterns and determine which one would best fit your needs, then read up on that pattern. If you have more questions about which one to adopt, make a couple of attempts and post them as another question about which one seems most appropriate and if you are going about it properly. Chances are there wont be a single consensus, so you'll have to make the final decision yourself.

Sorry I couldn't answer your overall question. I hope the rest helps though.