Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I am learning PHP OOP. I am familiar with PHP, and limited use of OOP, but not writing OOP. I have a small successful website that uses large associative arrays but is now suffering performance problems because the arrays soak up memory (My teamarray contains 16k teams of the data below, plus more). I am rewriting the code to take advantage of OOP and hopefully achieve greater performance as a result. (A quick trial reduced memory use by 16x).

I have this class:

class team {
private $teamid;
private $name;
private $urlname;
private $mascot;
private $city;
private $stateid;
private $jv;
private $color1;
private $color2;
private $address;
private $zip;
private $lat;
private $lng;

function __construct($teamid) {
    global $my_db;
    $row = $my_db->query("SELECT * FROM `team` WHERE `teamid` = $teamid;")->fetchRow(MDB2_FETCHMODE_ASSOC);

    $this->set_teamid = $row['teamid'];
    $this->set_name = $row['name'];
    $this->set_urlname = $row['urlname'];
    $this->set_mascot = $row['mascot'];
    $this->set_city = $row['city'];
    $this->set_stateid = $row['stateid'];
    $this->set_jv = $row['jv'];
    $this->set_color1 = $row['color1'];
    $this->set_color2 = $row['color2'];
    $this->set_address = $row['address'];
    $this->set_zip = $row['zip'];
    $this->set_lat = $row['lat'];
    $this->set_lng = $row['lng'];
} 

public function get_teamid() {
    return $this->teamid;
}
private function set_teamid($new) {
    $this->teamid = $new;
}

public function get_name() {
    return $this->name;
}
private function set_name($new) {
    $this->name = $new;
}

public function get_urlname() {
    return $this->urlname;
}
private function set_urlname($new) {
    $this->urlname = $new;
}

public function get_mascot() {
    return $this->mascot;
}
private function set_mascot($new) {
    $this->mascot = $new;
}

public function get_city() {
    return $this->city;
}
private function set_city($new) {
    $this->city = $new;
}

public function get_stateid() {
    return $this->stateid;
}
private function set_stateid($new) {
    $this->stateid = $new;
}

public function get_jv() {
    return $this->jv;
}
private function set_jv($new) {
    $this->jv = $new;
}

private public function get_color1() {
    return $this->color1;
}
private function set_color1($new) {
    $this->color1 = $new;
}

public function get_color2() {
    return $this->color2;
}
private function set_color2($new) {
    $this->color2 = $new;
}

public function get_address() {
    return $this->address;
}
private function set_address($new) {
    $this->address = $new;
}

public function get_zip() {
    return $this->zip;
}
private function set_zip($new) {
    $this->zip = $new;
}

public function get_lat() {
    return $this->lat;
}
private function set_lat($new) {
    $this->lat = $new;
}

public function get_lng() {
    return $this->lng;
}
private function set_lng($new) {
    $this->lng = $new;
}

}

The team is just an object container storing all static data, so I have the set_ functions at private.

The team can be added (not sure if it should be extended? probably the team_class class would be a better choice for going into the game) into a game class (not yet written, where two teams compete, time, date, location, scores, etc), and also (probably extended) into a team_class class (also not yet written) where they get properties about the season they are competing in (sport, year, level, etc).

Can it be improved? Are there any glaring problems?

Since $stateid is just an ID reference to the state, should I store the state name as part of the object? Is it better to compare stateid or state?

Do I need to store the $teamid?

Is there a better way to handle the colorsX? These are just 2 hex code color strings.

Can I declare all the private properties like:

private $teamid, $name, ....;

I'm thinking I would store all the object handlers (perhaps up to a hundred, maybe thousands) in an array for working with them. Is there a better way?

$teamarray[$teamid] = new team($teamid);
$teamarray[$teamid]->get_name();

Finally, is there an good, easy "fiddle" supports databases?

share|improve this question
    
Q: Are you just now moving your 16k records into a database? i.e. was it all in memory or a text file or something else previously? Q: Where does $teamid come from in the statement $teamarray[$teamid] = new team($teamid); i.e. do you have another query/text tile that gets all teamid's then loops over loading the data? SQLite is probably the easiest 'fiddle' database : sqlite.org –  David Farrell Dec 12 '12 at 19:38
    
No, I've been using MySQL from the beginning. What I previously was doing was getting ALL 16k teams and stuffing them into an array. I did this for all the "configuration" data from the database. A single page load was therefore using ~128M of memory, which is the main reason to go to OOP and only get what I need, when I need it. $teamid comes from the "controller" basically saying "I need information about this teamid." –  MECU Dec 12 '12 at 19:46
add comment

2 Answers

up vote 1 down vote accepted

Okay, here's a preliminary tidying up of the code, with comments:

<?php

class team {
  # You can stream line the private fields
  private $teamid, $name, $urlname, $mascot, $city, $stateid, $jv;
          $color1, $color2, $address, $zip, $lat, $lng;

  # Avoid `global $my_db`, perhaps direct injection 
  # of the db class into the constructor?
  function __construct($teamid) {
      global $my_db;

      # $teamid is not safe in this query it would seem...perhaps consider prepared statements? 
      $row = $my_db->query("SELECT * FROM `team` WHERE `teamid` = $teamid;")->fetchRow(MDB2_FETCHMODE_ASSOC);

      # Let's clarify this...you're using functions as such:
      # $this->set_teamid($row['teamid']); 
      # We're not doing any variable checking, so why not 
      # just set the variable directly and get rid of a 
      # bunch of functions and function calls?
      $this->teamid   = $row['teamid'];
      $this->name     = $row['name'];
      $this->urlname  = $row['urlname'];
      $this->mascot   = $row['mascot'];
      $this->city     = $row['city'];
      $this->stateid  = $row['stateid'];
      $this->jv       = $row['jv'];
      $this->color1   = $row['color1'];
      $this->color2   = $row['color2'];
      $this->address  = $row['address'];
      $this->zip      = $row['zip'];
      $this->lat      = $row['lat'];
      $this->lng      = $row['lng'];
  } 


  # Below, we will remove all the set_* functions 
  # and concatenate all the get_* functions into 1 
  # switch function
  public function get($varname){
    switch($varname){
      case 'teamid'   : $return = $this->teamid; break;
      case 'name'     : $return = $this->name; break;
      case 'urlname'  : $return = $this->urlname; break;
      case 'mascot'   : $return = $this->mascot; break;
      case 'city'     : $return = $this->city; break;
      case 'stateid'  : $return = $this->stateid; break;
      case 'jv'       : $return = $this->jv; break;
      case 'color1'   : $return = $this->color1; break;
      case 'color2'   : $return = $this->color2; break;
      case 'address'  : $return = $this->address; break;
      case 'zip'      : $return = $this->zip; break;
      case 'lat'      : $return = $this->lat; break;
      case 'lng'      : $return = $this->lng; break;
      default         : $return = "Error in get()."; break;
    }

    return $return;
  }

  # Replaced
  #public function get_teamid() {
  #    return $this->teamid;
  #} 

  # Removed
  #private function set_teamid($new) {
  #    $this->teamid = $new;
  #}

  # Replaced
  #public function get_name() {
  #    return $this->name;
  #}

  #Removed
  #private function set_name($new) {
  #    $this->name = $new;
  #}

  # Replaced
  #public function get_urlname() {
  #    return $this->urlname;
  #}

  # Removed
  #private function set_urlname($new) {
  #    $this->urlname = $new;
  #}

  # Replaced
  #public function get_mascot() {
  #    return $this->mascot;
  #}

  # Removed
  #private function set_mascot($new) {
  #    $this->mascot = $new;
  #}

  # Replaced
  #public function get_city() {
  #    return $this->city;
  #}

  # Removed
  #private function set_city($new) {
  #    $this->city = $new;
  #}

  # Replaced
  #public function get_stateid() {
  #    return $this->stateid;
  #}

  # Removed
  #private function set_stateid($new) {
  #    $this->stateid = $new;
  #}

  # Replaced
  #public function get_jv() {
  #    return $this->jv;
  #}

  # Removed
  #private function set_jv($new) {
  #    $this->jv = $new;
  #}

  # Replaced
  #public function get_color1() {
  #    return $this->color1;
  #}

  # Removed
  #private function set_color1($new) {
  #    $this->color1 = $new;
  #}

  # Replaced
  #public function get_color2() {
  #    return $this->color2;
  #}

  # Removed
  #private function set_color2($new) {
  #    $this->color2 = $new;
  #}

  # Replaced
  #public function get_address() {
  #    return $this->address;
  #}

  # Removed
  #private function set_address($new) {
  #    $this->address = $new;
  #}

  # Replaced
  #public function get_zip() {
  #    return $this->zip;
  #}

  # Removed
  #private function set_zip($new) {
  #    $this->zip = $new;
  #}

  # Replaced
  #public function get_lat() {
  #    return $this->lat;
  #}

  # Removed
  #private function set_lat($new) {
  #    $this->lat = $new;
  #}

  # Replaced
  #public function get_lng() {
  #    return $this->lng;
  #}

  # Removed
  #private function set_lng($new) {
  #    $this->lng = $new;
  #}
}

?>

And here it is without comments:

<?php

class team {
  private $teamid, $name, $urlname, $mascot, $city, $stateid, $jv;
          $color1, $color2, $address, $zip, $lat, $lng;

  function __construct($teamid) {
      global $my_db;
      $row = $my_db->query("SELECT * FROM `team` WHERE `teamid` = $teamid;")->fetchRow(MDB2_FETCHMODE_ASSOC);

      $this->teamid   = $row['teamid'];
      $this->name     = $row['name'];
      $this->urlname  = $row['urlname'];
      $this->mascot   = $row['mascot'];
      $this->city     = $row['city'];
      $this->stateid  = $row['stateid'];
      $this->jv       = $row['jv'];
      $this->color1   = $row['color1'];
      $this->color2   = $row['color2'];
      $this->address  = $row['address'];
      $this->zip      = $row['zip'];
      $this->lat      = $row['lat'];
      $this->lng      = $row['lng'];
  } 

  public function get($varname){
    switch($varname){
      case 'teamid'   : $return = $this->teamid; break;
      case 'name'     : $return = $this->name; break;
      case 'urlname'  : $return = $this->urlname; break;
      case 'mascot'   : $return = $this->mascot; break;
      case 'city'     : $return = $this->city; break;
      case 'stateid'  : $return = $this->stateid; break;
      case 'jv'       : $return = $this->jv; break;
      case 'color1'   : $return = $this->color1; break;
      case 'color2'   : $return = $this->color2; break;
      case 'address'  : $return = $this->address; break;
      case 'zip'      : $return = $this->zip; break;
      case 'lat'      : $return = $this->lat; break;
      case 'lng'      : $return = $this->lng; break;
      default         : $return = "Error in get()."; break;
    }

    return $return;
  }
}

?>

Again - the glaring issue is that SQL query...can easily be injected if you're not doing any checking in query() itself - and even if this is say, an intranet site, don't hedge on that for safety and laziness. Protect the database at all costs.

Now for your questions:

Can it be improved? Are there any glaring problems?

See above notes about SQL

Since $stateid is just an ID reference to the state, should I store the state name as part of the object? Is it better to compare stateid or state?

Feel free to store it, or not. Doesn't matter either way. If you require it for your software, then keep it obviously.

Do I need to store the $teamid?

Again, you don't need to if your software doesn't require it, but down the line, it might become a reference in some piece of software or other.

Is there a better way to handle the colorsX? These are just 2 hex code color strings.

Perhaps an array - ie: $colors[1]; $colors[2];? Or you can leave them as is.

EDIT

Edited for idiom:

<?php

class team {
  private $row;

  function __construct($teamid) {
      global $my_db;
      $this->row = $my_db->query("SELECT * FROM `team` WHERE `teamid` = $teamid;")->fetchRow(MDB2_FETCHMODE_ASSOC);
  }

  public function get($item){
    return $this->row[$item] ? $this->row[$item] : NULL;
  }
}

# Usage: 
$t = new team(14);
echo $t->get('city');

?>
share|improve this answer
    
Thanks for the tips. What does this mean? How would I do this? perhaps direct injection of the db class into the constructor? –  MECU Dec 11 '12 at 16:54
    
@MECU - rather than trying to explain it here, I'll just link to net.tutsplus.com/tutorials/php/dependency-injection-huh :) one of the best and simplest explanations I've found. Basically, instead of adding a global $db - you just pass the $db object to the class as you would the $teamid. –  jsanc623 Dec 11 '12 at 18:12
    
-1 for the getter pattern, which seems far from idiomatic - If you're going to get via a function like this, then why not just store $row directly and make get() check $this->row for the value and throw an error (or return null) if it doesn't exist? If you want to avoid having a bunch of setter/getter methods, you can implement ArrayAccess and then access the data as $myTeam['name']; If you want the data to be read-only then have the set* methods throw an Exception if called. Just the same, your IDE can generate your getter methods for you and then you can manage incremental changes. –  David Farrell Dec 12 '12 at 0:07
    
@DavidFarrell using array access where it's not necessary doesn't seem like the best advice - what's wrong with __get ? I agree about the null-benefit team::get() method in the answer though –  AD7six Dec 12 '12 at 0:24
    
@AD7six - When I use an array as backing-store, I use ArrayAccess for magic methods. When using magic methods on properties, I use _get - I basically do this to ensure 1-to-1 conformance to acceptable key values i.e. array keys can contain characters that php variables names cannot. It can also come down to preference on accessing, i.e. $myTeam['key'] vs $myTeam->key - Also if using a variable for key I tend to prefer $myTeam[$key] over $myTeam->$key –  David Farrell Dec 12 '12 at 1:02
show 3 more comments

Use oop to describe your domain, provide encapsulation, make code testable, etc. Oop does not inherently make code run more efficiently. An object with 13 properties uses just as much memory (if not slightly more) than an array with 13 keys.

My approach to oop is to describe the problem domain first. Worrying about the database at this point will only complicate things.

Obviously, I don't understand your problem domain but, for discussion purposes, let's say I have these things: Team, Season, Competition

Now, thinking about an api or how these things interact with each other you might have something like this to register teams for a season:

$fireBalls = new Team();
$fireBalls->setName('Fire Balls');

$springSeason = new Season();
$springSeason->setName('spring');
$sprintSeason->setYear(2013);
$springSeason->registerTeam($fireBalls);

To set up a game perhaps:

$game1 = new Game();
$game1->setTime(new DateTime('2012-03-01 15:00:00'));
$game1->setTeams($fireBalls, $dragons); //these two teams already created/fetched

There is much omitted here but the point is to understand how your objects relate to one another and what kind of behavior is needed. Spend some time writing this all out on a whiteboard or paper. Talk it over with others who understand the domain. Make sure it properly describes the domain before writing a single line of actual code. Note that a model is not simply Entities with a bunch of setters/getters. The model should contain actual behavior (such as registerTeam) which usually does more than just set 1 internal value. Behavior modifies the state of the model by performing calculations, checking validity, etc.

Once you are ready to persist some of this data, I highly recommend that you use an existing persistence library such as Doctrine, Propel, etc. Some of these work differently from one another but the idea is that they capture the state of your Model by mapping your Objects to your Relational Database and re-create your Objects from the Relational Database when you need them again.

Regarding some of your specific questions:

Are there any glaring problems? I'm not a huge fan of sticking persistence stuff into my Entities but, this is personal preference. However, if you want to use this pattern (called Active Record btw), the db object should be injected as others have already stated. Also, with passing an id to the constructor, how would one go about creating a new Entity?

All of your setters are private. Is the team class considered read only?

Since $stateid is just an ID reference to the state, should I store the state name as part of the object? It depends on whether you consider State to be a simple property of team or whether it is actually part of some related entity such as Address or Location.

Do I need to store the $teamid? If you use a ORM, let it worry about that.

Is there a better way to handle the colorsX? Handle it vertically instead of horizontally. i.e.

$team->addColor(1, 'red');
$team->addColor(2, 'green');

I'm thinking I would store all the object handlers (perhaps up to a hundred, maybe thousands) in an array for working with them. Is there a better way? This sounds like the source of your original problem! Large arrays can chew up alot of memory. It does not matter if it's an array of arrays or an array of objects. A Relational Database is much better for the job. There are many to choose from, postgresql, mysql, sqlite, oracle, etc

share|improve this answer
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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