OOP: Passing DB Conn to another object

[mich2004]

Hi Guys, I'm new to OOP. I've mastered some basic syntax, but am wondering about an issue of design. To clarify in this example, I'm simply looking to pull Team data from a Database (I've not included my db class (PDO wrapper) although I know it is working.

Although I have seen this sort of method below in a book (I believe and on the net) and it does work, I cannot help thinking coupling the DB class so tightly with the Team Class isn't a great approach. Can anyone give me feedback as to whether my approach is valid? While I can see the negative issue of tight coupling (i.e. changes to any database method would require multiple changes in Team (as team methods were added), I cannot really see an alternative way of doing this? I guess this is an issue of a design pattern or more advance OOP. Can anyone suggest an alternative way and/or more reading on making the coupling looser while still achieving my objectives?

Should, for example, all DB functionality in getName be done during implementation? I initally thought pushing all db related functionality inside a Team method was wise and the best way then to add further methods, i.e. getResults method would be similar to getName but obviously with a different query and processing afterwards inside Team, but now I wonder if all should be in the implementation, or is there another approach?

Thanks in advance.

// create team class
class Team {
    
    private $db;
    private $team_name;
    private $team_id;
    private $result;
    
    // in constructor lets pass DB object
    public function __construct($db) {    
        $this->db = $db;
        }
    
    //function to get team name
    public function getName($team_id) {
        $this->team_id = $team_id;    
        
        $this->db->query("SELECT team_name, nickname, founded FROM club WHERE team_id=:teamid");
        
        $this->db->bind(':teamid', $this->team_id);        
        
        if ($result = $this->db->single());
        {        
        return $result[];
        }
        
        }
}// end class


// Implementation
$team_id=1; //passed from user input

// First Create Db Object with correct passed variables
$database = new Database($server,$db_type);

// Invoke DB connect method
$database->connect();

// Now create Team object, pass it database object
$team = new Team($database);

// call Team method
$team_display = $team->getName($team_id);

// close db
$database->closedb();

[adam_bray]

Have a read up on using a singleton design with your classes.

 

I've applied it to your current code and removed the unnecessary implementations. Each constructor checks whether the class is already active, if it isn't then the class is constructed and stored.

 

One function that's missing from this is the autoload function, something else to read up on. 

<?php

class Database {
	
	// Store the class instance
	private static $instance = null;

	private function __construct() {
		// Add DB connection here
	}
	
	// Use a function to run the class constructor
	// Check if the class has already been initiated
	// Return class object
	public static function getInstance() {
		if( !isset( self::$instance ) ) {
			self::$instance = new Database;
		}
		return self::$instance;
	}
	
}
	
	// create team class
class Team {
    
	// Store the class instance
	private static $instance = null;
	
	// DB object
    private $db = null;
	
    private $team_name;
    private $team_id;
    private $result;
    
    // Check if the DB is active
	// If not get the DB instance
    private function __construct() {    
		if( !isset( $this->db ) ) {
			$this->db = Database::getInstance();
		}
  	}
	
	// Singleton function
	// Call the constructor if the class hasn't been created
	public static function getInstance() {
		if( !isset( self::$instance ) )	{
			self::$instance = new Team;	
		}
		return self::$instance;
	}
    
    //function to get team name
    public function getName( $team_id ) {
        
		$this->team_id = $team_id;    
		
		$this->db->query( "SELECT team_name, nickname, founded FROM club WHERE team_id=:teamid" );
		$this->db->bind( ':teamid', $this->team_id );        
		
		if( $result = $this->db->single() );
		{        
			return $result[];
		}
		return false;
	}
}// end class

// Get the team id
$team_id = 1;

// Build the team object
$team = Team::getInstance();

// call Team method
$team_display = $team->getName($team_id);

?>

I hope this answers your question.

[ignace]

I cannot help thinking coupling the DB class so tightly with the Team Class isn't a great approach.

You are correct. The correct way of doing it is by

 

1) using doctrine to do your table to object mapping:

http://docs.doctrine-project.org/en/2.0.x/reference/xml-mapping.html#example

 

2) or (and this is the lesser option) writing your own mapper:

http://www.sitepoint.com/integrating-the-data-mappers/#highlighter_536159

 

I would not follow adam_bray's advice, because

 

1) it couples your entity to the database

2) it introduces globals (you can no longer control who has access to the database, even your View can start executing queries)

3) it uses a singleton, which is an anti-pattern

4) in those rare cases you need to work with more then 1 database connection you have a problem.

 

--

Keeping your entities decoupled from the database means that they will have more cohesive methods and less clutter (db specific methods like find*() and save() or something).

Edited May 17, 2014 by ignace

[Strider64]

While ignace in my opinion is a more elegant and better solution(s), I think the following would prevent duplication if you added this to the adam_bray's class:

    // Empty clone magic method to prevent duplication:
    private function __clone() {
        
    }

Edited May 17, 2014 by Strider64

[KevinM1]

The main issue is that singletons are Bad. They're just another global which leads to tight coupling, which leads to rigid code, which leads to spaghetti-flavored hacks to work around it.

 

Singletons are wholly unnecessary in PHP where a script's lifespan generally lasts just for one request and objects are passed by reference by default. Generally, if you think you need a singleton, what you really want is dependency injection.

Edited May 17, 2014 by KevinM1

[mich2004]

Have a read up on using a singleton design with your classes.

 

I've applied it to your current code and removed the unnecessary implementations. Each constructor checks whether the class is already active, if it isn't then the class is constructed and stored.

 

One function that's missing from this is the autoload function, something else to read up on. 

<?php

class Database {
	
	// Store the class instance
	private static $instance = null;

	private function __construct() {
		// Add DB connection here
	}
	
	// Use a function to run the class constructor
	// Check if the class has already been initiated
	// Return class object
	public static function getInstance() {
		if( !isset( self::$instance ) ) {
			self::$instance = new Database;
		}
		return self::$instance;
	}
	
}
	
	// create team class
class Team {
    
	// Store the class instance
	private static $instance = null;
	
	// DB object
    private $db = null;
	
    private $team_name;
    private $team_id;
    private $result;
    
    // Check if the DB is active
	// If not get the DB instance
    private function __construct() {    
		if( !isset( $this->db ) ) {
			$this->db = Database::getInstance();
		}
  	}
	
	// Singleton function
	// Call the constructor if the class hasn't been created
	public static function getInstance() {
		if( !isset( self::$instance ) )	{
			self::$instance = new Team;	
		}
		return self::$instance;
	}
    
    //function to get team name
    public function getName( $team_id ) {
        
		$this->team_id = $team_id;    
		
		$this->db->query( "SELECT team_name, nickname, founded FROM club WHERE team_id=:teamid" );
		$this->db->bind( ':teamid', $this->team_id );        
		
		if( $result = $this->db->single() );
		{        
			return $result[];
		}
		return false;
	}
}// end class

// Get the team id
$team_id = 1;

// Build the team object
$team = Team::getInstance();

// call Team method
$team_display = $team->getName($team_id);

?>

I hope this answers your question.

 

Hi Adam, thanks for this, I do generally follow what you have written and I have managed to implement it as a test.

 

Can I ask one further question. Being passed to my implementation are two variables $server and $db_type, up to this point I have been passing these to the constructor of my DB class to determine the connection credentials required (i.e. different db or password for live/local server etc.), i.e. $database = new Database($server,$db_type).

 

However, If I wanted to pass the same variables now, I'm not sure how. It doesn't seem appropriate to pass these via the Team object at all. So what is the best way using the singleton method to be able to pass variables to the DB constructor, is this even possible or advised? Your help would be appreciated.

[mich2004]

I wanted to thank everyone for the help. Seems there is a lot more reading for me to do. While I do understand Adam's singleton approach, I must admit some of the other methods are over my head at this stage. I'll go and read some more.

[Jacques1]

No offense, but claiming to aim for decoupling and then going with singletons makes absolutely no sense whatsoever. Singletons are the most extreme form of coupling, because now you have hard-coded references to one specific class everywhere. It's impossible to use your classes with any other database interface – unless you literally go through your entire code and manually change those references.

 

What makes even less sense is that your original approach was actually pretty decent and much better in terms of decoupling. So you're replacing good code with bad code (as to your declared goals and general consensus). What's the point of that?

 

I don't get it.

[sKunKbad]

The main issue is that singletons are Bad. They're just another global which leads to tight coupling, which leads to rigid code, which leads to spaghetti-flavored hacks to work around it.

 

Singletons are wholly unnecessary in PHP where a script's lifespan generally lasts just for one request and objects are passed by reference by default. Generally, if you think you need a singleton, what you really want is dependency injection.

 

Doesn't OP's code more or less show an example of dependency injection? Yes, having the class and implementation of the class in the same script is not appropriate, but he's learning, and it's not half bad. Also, I think a DB connection is a great place for a singleton, no? Would one really want to make multiple connections to the database in the same request?

 

In any case, OP needs to read up on dependency injection and service locators, and also take a look at the modern PHP frameworks and how they are handling dependencies.

[mich2004]

Guys, as you can imagine I'm pretty confused by now with so many replies suggesting so many different things.

 

Can we start again. Is my initial code on the right tracks? I don't quite follow the "having the class and implementation of the class in the same script is not appropriate" comment.

 

I can see the coupling of the class is too tight, but to be honest I'm just lost from there with so many suggestions making different points!

[ignace]

Is my initial code on the right tracks?

No, it couples your entity Team class to the database. You have then 2 options:

 

1) use an ORM like Doctrine which promotes loosely coupled entities.

http://docs.doctrine-project.org/projects/doctrine-orm/en/latest/reference/configuration.html

 

2) write your own mapper but soon find yourself cursing at its limitations.

http://www.sitepoint.com/integrating-the-data-mappers/#highlighter_536159

 

With Doctrine it's easy to have a rich domain model (which should be your goal).

 

namespace My\Model\Entity;

class Team {
  private $id;
  private $name;
  
  // methods
}

Since I don't know what it is you are building. I am gonna assume you also have Player's:

 

namespace My\Model\Entity;

class Player {
  private $id;
  private $name;
  private $team;
  
  // methods
}

And that Player can join or leave a Team (business rule: can only join if not already part of a team):

 

// Player
public function join(Team $team) {
  if ($this->team !== null) {
    throw new \DomainException('You should leave the other team first...');
  }
  
  $this->team = $team;
  $this->team->add($this);

  return $this;
}

public function leave() {
  $this->team->remove($this);
  $this->team = null;
  
  return $this;
}

public function isMemberOf(Team $team) {
  return $this->team === $team;
}

$beginners = new Team('Beginners');
$advanced = new Team('Advanced');

$player->join($beginners)->leave()->join($advanced);

if ($player->isMemberOf($advanced)) {
 // advanced player
}

A team can compete in a tourney:

 

// Team
public function competeIn(Tournament $tournament) {
  $tournament->registerTeam($this);
}

To store it in the database you use an EntityManager:

 

$em = EntityManager::create($dbParams, $config);

$em->persist($player); // cascades all changes to the database
$em->persist($tournament);
$em->flush();

No clutter in all of these methods. Complete unaware of a database it exposes your domain, and how it works. But this probably sounds very rocket-sciencey to you. So maybe you can define what it is you are trying to build and we can give you better examples. Edited May 18, 2014 by ignace

[KevinM1]

Also, I think a DB connection is a great place for a singleton, no? Would one really want to make multiple connections to the database in the same request?

No, it's not a great place for a singleton. Remember, objects are passed by reference by default. If you create a database object (say, a PDO instance), and pass it around to other objects or methods, only references to that single instance are passed around. You're not passing multiple copies/connections around. It's just references to a single resource. That's why dependency injection is the way to go. You get your single instance, but without blowing apart scope/encapsulation to get it.

 

Regarding the OP's code, yes, he used dependency injection, which is great. My comments were aimed towards adam_bray, who not only placed the database in a singleton, but also the Team, which is definitely not the way to go. His suggestion strikes me as coming from someone who only has a hammer, and thus sees everything as a nail.

 

---

 

@mich2004 - Ignace had your solution. You need to use an ORM (Object Relationship/Relational (I've seen both) Mapper). It's what maps your database tables to an object representation (usually referred to as an Entity). You can either use an existing solution (his link to Doctrine 2, which is probably the most popular and widely used), or roll your own (the Sitepoint link). The point is that your Team object is supposed to be ignorant of the db. The mapper should do the heavy lifting of populating/editing/saving Team info. The Team object should be an in-code representation of your Team(s) and little else.

[Jacques1]

Guys, do you realize that mich2004 is new to OOP and already totally confused? What's the point of insisting on advanced stuff like ORMs?

 

By all means, his code is fine. Yes, there are all kinds of fancy PDO wrappers, ORMs, frameworks and whatnot. But it's not like this would somehow magically improve your code. In fact, I think this has become a fetish where people look down at decent programmers just because plain SQL is no longer considered cool.

 

The only dependency in his code is the dependency on an SQL database. Well, I think he can live with that for now.

[KevinM1]

Guys, do you realize that mich2004 is new to OOP and already totally confused? What's the point of insisting on advanced stuff like ORMs?

 

By all means, his code is fine. Yes, there are all kinds of fancy PDO wrappers, ORMs, frameworks and whatnot. But it's not like this would somehow magically improve your code. In fact, I think this has become a fetish where people look down at decent programmers just because plain SQL is no longer considered cool.

 

The only dependency in his code is the dependency on an SQL database. Well, I think he can live with that for now.

Well, he did come here asking how to solve that particular problem of his Team object being tightly coupled to his db wrapper. And Ignace answered him. The problem is that sprinkled among Ignace's answer were suggestions that would do more harm than good. Which is what I was trying to address.

 

Whether or not an ORM adds too much complexity is up for mich2004 to decide. But our position, as a site, is to inform and educate best practices. Ignace's answer was perfect because it provided two avenues:

 

1. Slap an enterprise-level solution into your code and call it a day.

 

or

 

2. Learn how to do it yourself on a smaller scale so you understand the mechanics behind it.

 

I don't see anything wrong with that.

 

And OOP is confusing at first. It took me a long time to understand it. But I don't think withholding information, when it's something specifically asked for, is the way to go. "It's good enough" isn't what we do here when someone asks how to address a problem.

[Jacques1]

If you add an ORM, you simply replace the dependency on a DB wrapper with a dependency on a certain ORM interface. Instead of query(), fetch() etc., you now have load(), save() and whatnot.

 

Don't get me wrong, I have nothing against ORMs or ignace's suggestions. ORMs can be a useful abstraction layer in certain scenarios. What I don't like is when they're thrown at “newbies” as the one-and-only solution which absolutely must be used.

 

No, you don't need an ORM for good OOP code. A simple database interface is perfectly fine. You may be flamed for not being cool or enterprisey enough, but those are social matters, not technical ones.