Is it a good idea to define one big private function in a class to maintain valid state, that is, to update the object's data members?

Question

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.

Answer 1

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.

Answer 2

Any disadvantages[?]

Sure. This method relies on everyone always remembering to do something. Any method relying on everyone&always is bound to fail sometimes.

maybe better ways to do this?

One way to avoid the burden of remembering ceremony is calculating properties of the object that depend on other properties as needed, as @eigensheep suggested.

Another is making the cart item immutable and calculating it in the constructor/the factory method. You normally would go with "calculate as needed" method, even if you made the object immutable. But if the calculation is too time consuming and would be read many, many times; you may choose "calculate during creation" option.

$i = new CartItem();
$i->setPrice(100);
$i->setShipping(20);

You should ask yourself; Does a cart item without a price makes sense? Can the price of an item change? After it is created? After its tax calculated? etc Maybe you should make CartItem immutable and assig price and shipping in the constructor:

$i = new CartItem(100, 20);

Does a cart item make sense without the cart it belongs to?

If not, I'd expect $cart->addItem(100, 20) instead.