Coupon code manager

My concern lies in understanding the nature of an object. My reasoning: when I instantiate my class, I am either creating an entirely new coupon, or I am referencing an existing one from the database. So I am thinking that the new TJ_Coupon actually is a new coupon. Am I thus using the constructor correctly?

And then my other question is about abstraction. How far should I abstract? I have my setters which then go through three functions to actually change the DB. I am interested in feedback about that flow (seen near the end of the document and moving up).

```
/**
* Class TJ_Coupon
* Exceptions
* Codes:
* 0 -> Code Wrong Length
* 1 -> Bad Date
* 2 -> Wrong Discount Format
* 3 -> Uses not positive integer
* 4 -> Coupon code already exists
* 5 -> Coupon code does not exist
* 6 -> Coupon has already been used
* 7 -> Coupon code is not for current user
* 8 -> WP User ID is not valid
* 9 -> Coupon can only be used once per order
*/
class TJ_Coupon
{
/**
* @var string
*/
protected $name;
/**
* @var string
*/
protected $code;
/**
* @var string
*/
protected $discount;
/**
* With either a $ prefix or a % suffix
* @var string
*/
protected $expiration_date;
/**
* @var int
*/
protected $uses;
/**
* @var int WP_User ID
*/
protected $user;

/**
* @param $code string coupon code
* @param $create bool do you want to update database or just instantiate
* @param $new_coupon array
* @throws Exception
*/
function __construct($code, $create = false, $new_coupon = array())
{
$coupons = get_option('tj_coupons', array());

if ($create) {
$name = $new_coupon['name'];
$code = $new_coupon['code'];
$discount = $new_coupon['discount'];
$expiration_date = $new_coupon['expiration_date'];
$uses = $new_coupon['uses'];
$user = $new_coupon['user'];

I'm not seeing blatant non-OOPness, but there is a little funkiness here:

-
The setters ideally should be usable -- and should be used -- everywhere you import potential user input into the object. Since they validate the data before modifying anything, if you use them consistently, you can ensure the coupon's fields are valid without actually having to check if they are every time.

The problem is that your setters update the database. Besides the fact that modifying all the fields of a coupon means updating the tj_coupons option 6 times, things go sideways if i start with a new coupon and don't set code instantly. The functionalities of setting fields in the coupon and updating the database, should be separated and easily distinguishable. Perhaps have a public save() method or something.

-
Your constructor is doing more work than it should. All it should really do is set stuff up. You might consider splitting up the functionality to add a new coupon vs retrieving an old one.

protected function __construct($coupon) {
    $this->name            = $this->check_name($coupon['name']);
    $this->code            = $this->check_code($coupon['code']);
    $this->discount        = $this->check_discount($coupon['discount']);
    $this->expiration_date = $this->check_expiration_date($coupon['expiration_date']);
    $this->uses            = $this->check_uses($coupon['uses']);
    $this->user            = $this->check_user($coupon['user']);
}

public static function withCode($code) {
    $coupons = get_option('tj_coupons', array());
    $C_code = 'C_' . $coupon['code'];

    if (!isset($coupons[$C_code]))
        throw new Exception ('Coupon code does not exist', 5);

    return new TJ_Coupon($coupons[$C_code]);
}

public static function create($coupon) {
    $coupons = get_option('tj_coupons', array());

    # I personally prefer failing as early as possible.
    # Here, you fail the instant you see a bad coupon code,
    # rather than doing a bunch of work you're just going to toss out.
    $C_code = 'C_' . $coupon['code'];
    if (!isset($coupon['code']) or isset($coupons[$C_code]))
        throw new Exception ('Coupon code already exists', 4);

    $new_coupon = new TJ_Coupon($coupon);
    $new_coupon->update_coupon();
    return $new_coupon;
}

And then you'd use it like

$existing = Coupon::withCode('ABCD1234');

$created = Coupon::create(array(
    'code' => 'stuff',
    'name' => 'new coupon',
    ... other fields ...
));

-
Oh, and as has already been mentioned, $coupon->update_coupon() is redundant. Consider your method names as commands to the object or info about it. The object already knows what it is; you don't have to tell it. :)

-
The functions like check_user and check_discount, that do nothing more than validate a parameter against hard-coded rules, other parameters, or data unrelated to the instance, could be static.

-
An if/else to determine whether to return true or false is largely unnecessary. Boolean expressions evaluate to a value. Just return the expression you're comparing or testing. For example,

if (time() > strtotime($this->expiration_date))
    return true;
else
    return false;

can become

return time() > strtotime($this->expiration_date);

-
can_user_use($user_id) looks like it always returns false. (if ((condition)) return false; else return (condition); could only ever return true if condition evaluates to false the first time and true the second. If that's the case, $DEITY help the maintainer of this code.)

I might make it look like

public function can_user_use($user_id)
{
    return (!$this->for_user() or $user_id == $this->user);
}

if the logic is as i understand it.

-
check_discount seems more complicated than it has to be, and a bit error prone.

• It doesn't do much validation other than counting characters. The current validation allows for ugliness like 42$50 and %10$.

• Are the chances of someone putting two $ signs, or two % signs, really so high that it's worth having separate error messages for?

You should probably be checking against a regex rather than just counting chars. If the regex matches, and it's correct, you can safely assume the input format is valid. (You might still want to check that the number part is within a certain range, so you don't have 150% coupons. But you can know for sure that the discount looks correct.)

For example:

```
protected function check_discount($discount)
{
$ok = preg_match('/^(\$?)(\d*(?:\.\d+)?)(%?)$/', $discount, $matches);
# make sure there's a match, with a number and exactly one of $ or %
if (!($ok and $matches[2] and ($matches[1] xor $matches[3]))) {
throw new UnexpectedValueException(
"'$discount' doesn't match \$number or number%", 2
);
}

# sanitizing is no longer strictly necessary. It won't do anything useful,
# since we've already

StackExchange Code Review Q#28023, answer score: 5