OOP Design Issue...

[mich2004]

Hi guys, I don't know if I'm over complicating things here and cannot see the wood for the trees, but I seem to have got myself confused. What I'm simply trying to do is get teamName and teamresults of a team from a database.  The code will perhaps explain better than I can (note I haven't included the DB class which is fine).

class TeamCollection {
    protected $database;
    protected $teamid;
    protected $team_name;
    
    // pass db by dependency injection...
    public function __construct(Database $database) {
            $this->database = $database;    
    }

    public function getTeam($teamid){
            $this->team_id = $teamid;
            $this->database->query("SELECT  team_name, nickname, founded FROM club WHERE team_id=:teamid");
            $this->database->bind(':teamid', $this->team_id);    
            
        // spawn a new Team object if query is valid, if not throw exception and end via try/catch...
        if ($result = $this->database->single()) {
                return new Team($result); // create new Team passing $result to Team constructor
        }
        else {
                throw new exception ('No Valid Team returned');
        }
    }
    
    public function getResults($teamid){
            $this->team_id = $teamid;
            $this->database->query("SELECT result FROM results WHERE team_id=:teamid");
            $this->database->bind(':teamid', $this->team_id);    
    
        // If a Team has valid results, return array of match scores..
        if ($scores = $this->database->results()) {
                return $scores;
        }
        else {
            throw new exception ('No Valid Team Results to return');
        }
    }

} //end class

// Team is abstracted from DB
class Team {
    
    private $team_name;
    private $result;
    private $scores;
    
    public function __construct(array $result) {
        $this->team_name = $result['team_name'];
    }
    
    public function getName() {
        $this->team_name;
    }
    
    public function getResults(array $scores) {
        foreach ($scores as $row){
            echo $row;    
        }
    }
}
// implementation

$team_id =9; // passed variable..

$database = new Database($server,$db_type); // create db..

$database->connect(); // connect to db..

$collection = new TeamCollection($database); // pass live DB link to TeamCollection Class

try {
    
    $team = $collection->getTeam($team_id); // try and spawn a valid Team Object..

    echo $name = $team->getName(); // If so lets return a Team name

    $scores = $team->getResults($team_id); // Now try and get Team Results..

    $display_results = $scores->getResults($scores); // Display results by passing them to Team class to

    }

catch (Exception $e) {
    Echo 'Exception Thrown: ' . $e->getMessage();    
}

My basic difficulty is where to put the getResults method and how to use it? I'm fine spawning a new Team as required, but is my logic for then moving on to getResults sound? I was trying to abstract my Team class from having any database association at all. Is this a wise approach or just not required? Am I overcomplicating the issue or missing something? Can anyone help? I guess my difficulty is between connecting classes and objects in the best manner. Thanks in advance.

Edited May 19, 2014 by mich2004

[Jacques1]

Hi,

 

the whole approach doesn't make a lot of sense to me, and repeating everything in a new thread doesn't help.

 

First of all, why do you want to keep all queries out of the Team class? Is there a specific reason for this? After all, this is a database-related class, and no abstraction will change that. Unless you plan to give up SQL at some point and move to an entirely different database concept, hiding the SQL queries simply isn't necessary.

 

If you absolutely must do this, then we're back at ORMs. As the others have already said, trying to implement this on your own is nonsensical. Learning an ORM does require some time, so don't complain that this is all too complicated.

[Psycho]

A few other comments:

 

It doesn't make sense to have a Team object that only contains the name. It should contain all of the relevant details about the team (such as the results). No need to run multiple queries. Just decide what properties (if any) should be directly accessible as opposed to using setter/getter methods.

 

I also agree that the separate classes probably aren't needed.

 

 

       // spawn a new Team object if query is valid, if not throw exception and end via try/catch...
        if ($result = $this->database->single()) {
                return new Team($result); // create new Team passing $result to Team constructor
        }
        else {
                throw new exception ('No Valid Team returned');
        }

The else block is unnecessary here. If the if() condition passes, the 'return' will exit the method. Also, I think it is good practice to have a failure return false. So, this could be simplified as:

 

       // spawn a new Team object if query is valid, if not throw exception and end via try/catch...
        if ($result = $this->database->single()) {
                return new Team($result); // create new Team passing $result to Team constructor
        }
        throw new exception ('No Valid Team returned');
        return false;
       

[ignace]

First of all, why do you want to keep all queries out of the Team class?

Why do you keep persisting that he should?

 

1) It breaks class cohesion.

2) It adds an unnecessary dependency.

 

Team is not a database. There is no relation between Team and the database at all.

 

----

@OP Your Team class should have setters to set the data from the database onto your object.

 

namespace My\Model\Mapper {
  class TeamMapper {
    private $db;
    
    public function find($id) {
      $this->db->query("SELECT  team_name, nickname, founded FROM club WHERE team_id=:teamid");
      $this->db->bind(':teamid', $id);
      
      if ($row = $this->db->single()) {
        $team = new Team;
        $team->setId($id);
        $team->setName($row['name']);
        $team->setFoundedDate(new \DateTime($row['founded']));
        
        $this->db->query("SELECT result FROM results WHERE team_id=:teamid");
        $this->db->bind(':teamid', $id);
        if ($rows = $this->db->results()) {
          foreach ($rows as $row) {
            $team->addScore($row['result']);
          }
        }
      }
      return $team;
    }
  }
}

Edited May 19, 2014 by ignace

[JonnoTheDev]

There are lots of ways to accomplish what you are doing. Here's another method (although this is probably going to get some critics LOL)

<?php
class Team
{
	private $_db;
	private $_team_id;	
	private $_team_name;
	
	private function __construct($db, $team)
	{
		$this->_db = $db;
		$this->_team_id = $team->team_id;
		$this->_team_name = $team->team_name;
	}
	
	public static function get($db, $team_id)
	{
		$db->query("SELECT team_name, nickname, founded FROM club WHERE team_id=:teamid");
		$db->bind(':teamid', $team_id);
		if($result = $db->single())
		{
			return new Team($db, $result);
		}
		return FALSE;
	}
	
	public function name()
	{
		return $this->_team_name;	
	}

	public function get_results()
	{
		$this->_db->query("SELECT .....");
		// etc
	}
}


// usage
if($my_team = Team::get($database, 1))
{
	echo $my_team->name();
	print_r($my_team->get_results);
}

?>

Edited May 19, 2014 by neil.johnson

[mich2004]

Thanks all, I was having a bit of a head spinning day when I posted!

 

I'd sortof adapted a solution using Neil's method (passing db to constructor of team class), but of course the missing link for me was creating a static method and in turn making the team constructor private.