PHP Design Patterns – Defining a Large Private Function to Maintain Valid State

design-patternsobject-orientedPHPstate

Although in the code below a simple single item purchase in an e-commerce site is used, my general question is about updating all data members to keep an object's data in valid state at all times.

I found "consistency" and "state is evil" as relevant phrases, discussed here:
https://en.wikibooks.org/wiki/Object_Oriented_Programming#.22State.22_is_Evil.21

<?php

class CartItem {
  private $price = 0;
  private $shipping = 5; // default
  private $tax = 0;
  private $taxPC = 5; // fixed
  private $totalCost = 0;

  /* private function to update all relevant data members */
  private function updateAllDataMembers() {
    $this->tax =  $this->taxPC * 0.01 * $this->price;
    $this->totalCost = $this->price + $this->shipping + $this->tax;
  }

  public function setPrice($price) {
      $this->price = $price;
      $this->updateAllDataMembers(); /* data is now in valid state */
  }

  public function setShipping($shipping) {
    $this->shipping = $shipping;
    $this->updateAllDataMembers(); /* call this in every setter */
  }

  public function getPrice() {
    return $this->price;
  }
  public function getTaxAmt() {
    return $this->tax;
  }
  public function getShipping() {
    return $this->shipping;
  }
  public function getTotalCost() {
    return $this->totalCost;
  }
}
$i = new CartItem();
$i->setPrice(100);
$i->setShipping(20);
echo "Price = ".$i->getPrice(). 
  "<br>Shipping = ".$i->getShipping().
  "<br>Tax = ".$i->getTaxAmt().
  "<br>Total Cost = ".$i->getTotalCost();

Any disadvantages, or maybe better ways to do this?

This is a recurring issue in real-world applications backed by a relational database, and if you do not use stored procedures extensively to push all the validation into the database. I think that the data store should just store data, while the code should do all the run-time state maintaining work.

EDIT: this is a related question but does not have a best-practice recommendation regarding a single big function to maintain valid state:
https://stackoverflow.com/questions/1122346/c-sharp-object-oriented-design-maintaining-valid-object-state

EDIT2: Although @eignesheep's answer is the best, this answer – https://softwareengineering.stackexchange.com/a/148109/208591 – is what fills the lines between @eigensheep's answer and what I wanted to know – code should only process, and global state should be substituted by DI-enabled passing of state between objects.

Best Answer

All else being equal, you should express your invariants in code. In this case you have the invariant

$this->tax =  $this->taxPC * 0.01 * $this->price;

To express this in your code, remove the tax member variable and replace getTaxAmt() with

public function getTaxAmt() {
  return $this->taxPC * 0.01 * $this->price;
}

You should do something similar to get rid of the total cost member variable.

Expressing your invariants in your code can help avoid bugs. In the original code, the total cost is incorrect if checked before setPrice or setShipping is called.

Related Topic