1
\$\begingroup\$

I wrote this php class to generate one time random and unique coupon codes.

The Class generates coupon codes, storing it in a json file.

My question is regarding the write and get methods of this class.

In the write method, I use file_put_contents with LOCK_EX flag. I assume if the function fails to get a lock, it returns false.

When the Get method is called,

  • it pop a coupon code from the data array property
  • call the write method to update the file
  • if write returns true, it returns the coupon to the user and updates the database
  • if write returns false, it sleeps 10ms and makes a recursive call to itself.

Is this a correct way to prevent concurrent file access issues? For example, 2 users getting the same coupon code.

Any other suggestions or improvements would be much appreciated.

<?php
namespace Cart;
/**
* Generates random and unique coupon codes
*/
class CouponGenerator
{
  private $query;
  private $data;
  private $file;
  private $prefix;
  private $noOfCodesToGenerate = 10000;

  public function __construct(\QueryBuilder $query, $prefix = '')
  {
      switch ($prefix) {
          case 'FB':
              $this->file = 'fb.json';
              break;

          default:
              $this->file = 'coupons.json';
              break;
      }

      $this->query = $query;

      // if file doesn't exist or empty
      if (! file_exists($this->file) || empty($this->data = $this->read())) {
          // generate code and write to file
          $this->generateCodes();
          $this->write();
      }
  }

  // returns the coupon code
  public function get()
  {
      // Pop the last coupon
      $code = array_pop($this->data);

      // try updating the file
      if ($this->write()) {
          // if successful update the database and return code
          $this->updateTable($code);
          $data = [];
          return $code;
      }
      // else sleep 10ms and attempt again
      usleep(100000);
      $this->get();
  }

  private function updateTable($code)
  {
      // update database and return result
  }

  private function read()
  {
      return json_decode(file_get_contents($this->file));
  }

  private function write()
  {
      return false !== file_put_contents($this->file, json_encode($this->data), LOCK_EX);
  }

  private function generateCodes() {
      $chars = '23456789ABCDEFGHJKLMNPQRSTUVWXYZ';
      $charsLen = strlen($chars) - 1;
      $code = $this->prefix;
      $maxLen = 8 - strlen($this->prefix);
      $result = [];

      while (count($result) < $this->noOfCodesToGenerate) {
          for ($i=0; $i < $maxLen; $i++) {
              $code .= $chars[mt_rand(0, $charsLen)];
          }

          if (! in_array($code, $result)) {
              $result[] = $code;
          }
          $code = $this->prefix;
      }
      $this->data = $result;
  }
}

EDIT I took @freezePhoenix advice and rewrote the below function. Its not as random and unique as a UUID but i ran a test for 100000 codes and got just 3 collisions.

To eliminate duplicate codes, i implemented a try catch block to catch the PDOException and regenerate the code.

private function generateCode() {
      $randomCode = $this->prefix . bin2hex(openssl_random_pseudo_bytes(16));
      $ambigousChars = [0, 'o', 1, 'i', 'l'];
      $cleanString = str_replace($ambigousChars, '', $randomCode);
      return strtoupper(substr($cleanString, 0, 8));
}

public function get()
{
    try {
        $code = $this->generateCode();
        $this->updateTable($code);
        return $code;
    } catch (PDOException $e) {
        $this->get();
    }
}
\$\endgroup\$
  • 1
    \$\begingroup\$ Why don't you use UUID? Or the general barcode? \$\endgroup\$ – FreezePhoenix Aug 29 '18 at 12:58
  • 1
    \$\begingroup\$ You mean like uniqid (php.net/manual/en/function.uniqid.php). That should help avoid all of the above issues and avoid all the performance issues. I will give it try. Thanks \$\endgroup\$ – Benny Thadikaran Aug 29 '18 at 13:17
  • \$\begingroup\$ No... just write your own UUID generator. \$\endgroup\$ – FreezePhoenix Aug 29 '18 at 13:23
  • \$\begingroup\$ @FreezePhoenix - I wasn't aware of UUID but i did plenty of reading. Looks like it could be quite useful. However a 32 character coupon code is too long, so i just decided to go with bin2hex(openssl_random_pseudo_bytes(16)) . I removed all ambiguous characters and returned the first 8 characters as the coupon code. Your suggestion really simplified my code a good deal and perhaps even improved performance too. Not having to repeatedly read/write to the file system. Thanks \$\endgroup\$ – Benny Thadikaran Aug 30 '18 at 14:36

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Browse other questions tagged or ask your own question.