18

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.

  • I avoid having variables that are percentages. You can accept a percentage from the user, or display one to a user, but life is much better if the program variables are ratios. – kevin cline Feb 24 '17 at 9:15
29

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.

  • 3
    Many languages have getters so that such functions will pretend they are properties. The best of both! – curiousdannii Dec 24 '15 at 12:30
  • Excellent point, but my general use case is where code fetches and stores data to multiple columns in multiple tables in a relational database (MySQL mostly) and I don't want to use stored procedures (debatable and another topic on its own). Taking your invariants-in-code idea further this means that all calculations must be "chained": getTotalCost() calls getTaxAmt() and so on. This means we only ever store non-calculated things. Are we moving a bit towards functional programming? This also complicates storage of calculated entities in tables for quick access... Needs experimentation! – site80443 Dec 25 '15 at 9:04
13

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.

  • 3
    You point out the biggest disadvantage: relying on people remembering to do things is rarely a good solution. About the only thing you can rely on a human to do is that they will forget to do something. – corsiKa Dec 24 '15 at 22:04
  • @corsiKlause Ho Ho Ho and abuzittin, solid point, cannot argue with that - people forget invariably. However, the code I wrote above is just an example, there are substantial use cases where some data members are updated later. The other way I see is to normalise further - make classes such that independently updated data members are in other classes and push the responsibility of updation onto some interfaces - so that other programmers (and yourself after some time) have to write a method - the compiler reminds you that you have to write it. But that would add a lot more classes... – site80443 Dec 25 '15 at 8:51
  • 1
    @site80443 Based on what I see, that's the wrong approach. Try to model your data such that only data that is validated against it self is included. For example the price of an item can't be negative only relies on itself. If an item is discounted, don't factor the discount into the price - decorate it with a discount later. Store the $4.99 for the item and the 20% discount as a separate entity, and the 5% tax as yet another entity. It actually looks like you should consider the Decorator pattern if the examples represent your real life code. – corsiKa Dec 25 '15 at 17:04

Your Answer

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

Not the answer you're looking for? Browse other questions tagged or ask your own question.