Madproject

Always know where your towel is.

Refactoring a user input generated SQL query

I’m sure many of your are familiar with PHPMyAdmin. In short it’s a nice frontend to MySQL written in PHP that enables you to visually manipulate your MySQL databases and underlying data.

The reason I mention PHPMyAdmin is I recently needed to add a similar feature of PHPMyAdmin into one of my applications. The feature being: generate an SQL query based on users input from checkboxes.

The idea is simple: I want the user to be able to filter what type of orders will be displayed on the orders page by their payment status (Paid, Pending, Failed, Refunded)

When I’m not entirely clear on how to approach a problem programatically I tend to write it out in it’s longest version then refactor:

The very long winded and definitely not the best approach:

$switch = false;
if(!isset($payment_status['paid']) || !isset($payment_status['pending']) || !isset($payment_status['failed']) || !isset($payment_status['refunded'])) { //One of them is unset so we can act
  if(isset($payment_status['paid']) && $switch == false) {
    $switch = true;
    $sql .= " AND (orders.payment_status = 'paid'";
    if(isset($payment_status['pending'])) {
      $sql .= " OR orders.payment_status = 'pending'";
    }
    if(isset($payment_status['failed'])) {
      $sql .= " OR orders.payment_status = 'failed'";
    }
    if(isset($payment_status['refunded'])) {
      $sql .= " OR orders.payment_status = 'refunded'";
    }
    $sql .= ")";
  }
  if(isset($payment_status['pending']) && $switch == false) {
    $sql .= " AND (orders.payment_status = 'pending'";
    if(isset($payment_status['failed'])) {
      $sql .= " OR orders.payment_status = 'failed'";
    }
    if(isset($payment_status['refunded'])) {
      $sql .= " OR orders.payment_status = 'refunded'";
    }
    $sql .= ")";
  }
  if(isset($payment_status['failed']) && $switch == false) {
    $sql .= " AND (orders.payment_status = 'failed'";
    if(isset($payment_status['refunded'])) {
      $sql .= " OR orders.payment_status = 'refunded'";
    }
    $sql .= ")";
  }
  if(isset($payment_status['refunded']) && $switch == false) {
    $sql .= " AND orders.payment_status = 'refunded'";
  }
}

This solution basically enumerates each possibility and then adds the appropriate SQL query to the $sql variable that will then later be ran against the DB.

This is a poor solutions for many reasons, but chief amongst them:

  1. Very long winded and repetitive which goes against the DRY principle
  2. Each time you add a new payment status the code snippet would grow down the line and it wouldn’t be long before it would be unmaintanable

The next refactored version looks a bit better:

if(!isset($payment_status['paid']) || !isset($payment_status['pending']) || !isset($payment_status['failed']) || !isset($payment_status['refunded'])) { //One of them is unset so we can act  
  $sql .= " AND (0 = 1";
  if (isset($payment_status['paid'])) {
      $sql .= " OR orders.payment_status = 'paid'";
  if (isset($payment_status['pending'])) {
      $sql .= " OR orders.payment_status = 'pending'";
  }
  if (isset($payment_status['failed'])) {
      $sql .= " OR orders.payment_status = 'failed'";
  }
  if (isset($payment_status['refunded'])) {
      $sql .= " OR orders.payment_status = 'refunded'";
  }
  $sql .= ")";
}

What we’re doing here is we’re forcing the OR by adding 0 = 1 which will always return false. This then allows us to create a statement made completely with OR instead of AND like we were using in the first example which lead us to repeat ourselves. This is based on a truth table, check it out if you want more information.

Having said that, I think we can still do better!

Last and definitely not least we have:

$sql .= " AND orders.payment_status IN('".implode("','",array_keys($payment_status))."')";

Down from 37 lines to 1. Now that’s refactoring.

Let’s break it down:

  1. We use the MySQL operator IN() to Check whether a value is within a set of values
  2. We use PHP functions implode and array_keys to join the array keys(paid, pending, failed, refunded) into a string joined by a comma therefore building our IN() values to check

Say the user checked the paid and refunded boxes we’d get a query that would look like this:

 AND orders.payment_status IN('paid','refunded')";

Conclusion

Refactoring is cool!

Leave a Reply

Your email address will not be published. Required fields are marked *