Am I Passing Too Many Dependencies to Objects?

Hi! Been a few months since I last posted here. I was having an enlightening discussion going on in this topic in May and was never able to follow-up due to life happening.

Anyway, I’m back at this project in which I’m converting various parts of a custom PHP application from procedural to OOP code. I now understand that it’s better practice to pass dependencies to other objects that need them. But where I left off in that linked topic above is that I need to pass quite a few dependencies to some of my main classes for them to work. @Lemon_Juice asked this question that I wasn’t able to answer then…

Here’s why: I have a class called NewsList that loads a list of news articles from a database. Here’s how I instantiate the object:

$news = new NewsList($db, $site, $widget, $pager, $photo); 

$db, $site, $widget, $pager and $photo are all objects that have already been instantiated which the NewsList class needs (in the constructor).

function __construct(DB $db, Site $site, Widget $widget, Pagination $pager, Photo $photo)

Here’s a description of each dependency:

  • $db is the database object. NewsList needs to do at least one database query.

  • The $site object contains information about a client’s website settings that will be needed to build the database query.

  • The $widget object contains settings for the News widget which are also needed for part of the database query.

  • The $pager object will be updated by the NewsList class after the database query has been performed and will contain information used to create paginated lists.

  • The $photo object will also be updated by the NewsList class after the database query has been performed and will contain information about photos attached to each news article in the list.

So I need to pass that many object dependencies because NewsList is essentially a module that requires information from several other objects to build a database query to retrieve a list of news articles, and then after the query has run, two more objects ($pager and $photo) are updated with relevant information which I’ll need later to render the HTML page.

So if five dependencies is too many, what am I doing wrong in constructing my classes and objects? I have broken things down into smaller classes/objects, which is partially why I need to pass that many dependencies to the NewsList class.

1 Like

There is of course no absolute answer to how many dependencies are too many. Five is a bit of a red flag but I have seen more. The key (to me) is how maintainable your class is. Can you come back to it after a few months and still understand what you did? If so then I would not worry too much about it.

But are all of your arguments actually dependencies? $pager and $photo seem more like results to me. And is it really necessary to put everything into the constructor?

This is just off the top of my head but maybe you could do something like:

$newsList = new NewsList($db);

$newsList->query($site,$widget);

$pager = $newsList->getPager();
$photos = $newsList->getPhotos();

But again, if your current implementation have works for you then use it until it causes a problem.

4 Likes

It would be useful to see the code how you use your NewsList class - a simplified version.@ahundiak may have a point here saying that Pager and Photos are not needed to actually create a news list and to me this may indicate they form additional responsibilities of the class, whereas it would be good to limit the class to just one responsibility.

3 Likes

Okay, here’s the script for the NewsList class mentioned above:

class NewsList
{
	protected $module_name = "News List";
	protected $data = array();
	protected $content_link = '/news';

	// Dependencies

	private $db;
	private $site;
	private $widget;
	protected $pager;
	protected $photo; 
	protected $query; // loaded in constructor

	// Global properties

	protected $page_title = 'News';
	protected $see_all_label = "See All";
	protected $see_all_href = '';
	protected $number_get = 0;

	// List properties

	protected $show_expired = false;
	protected $sort_order = "DESC";

	function __construct(DB $db, Site $site, Widget $widget, Pagination $pager, Photo $photo)
	{
		$this->db = $db;			// Database object
		$this->site = $site;		// Site object, contains site settings
		$this->widget = $widget;	// Widget object, contains news widget settings
		$this->pager = $pager;		// Pagination object for creating paginated lists
		$this->photo = $photo;		// Photo object collects info about content photos
 
		$this->query = new Query; 	// Query object builds and returns a database query
	}

	/**
	 * --- SETTERS: There are a few methods in original script that set the values of some properties. 
	 */

	/**
	 * Retrieve list of news articles
	 * @return array
	 */
	public function retrieve()
	{
		// This calculates the starting row for LIMIT statement in query
		$this->pager->getStartRow();

		/**
		 * Build database query using Query object
		 */

		$this->query->table("news");
		$this->query->select("n.id, n.headline, n.teaser, n.pub_start, n.pub_end, n.url");
		$this->query->from("news n");
		$this->query->where("n.site_id = ".$this->site->id);

		if ($this->show_expired == false) {
			// Select news articles that haven't expired. 
			$this->query->where("UNIX_TIMESTAMP(pub_end) >= UNIX_TIMESTAMP(NOW())");
			$this->query->where("UNIX_TIMESTAMP(pub_start) <= UNIX_TIMESTAMP(NOW())");
		}

		$this->query->limit($this->pager->getLimit());
		$this->query->order('n.pub_start '.$this->sort_order);

		// Add parts to query for content photo

		$this->query->select("photo.status AS photo_status, photo.filename, photo.thumbname, photo.folder, photo.caption, photo.photog, photo.dim_x, photo.dim_y, photo.thumb_x, photo.thumb_y, photo.id AS photo_id");

		$this->query->from("LEFT JOIN photo ON photo.item_id = n.id AND photo.type_id = ".$this->site->getPhotoType['news']);

		// Get fully-constructed DB query
		$query = $this->query->getQuery();

		// Perform query and retrieve records
		$records = $this->db->select($query);

		// Get info about query
		$result = $this->db->queryInfo();

		// Set total rows and found rows for pagination links
		$this->pager->setHowmany($result['num_rows']);
		$this->pager->setTotalRecords($result['found_rows']);

		/**
		 * Build data array
		 */

		if ($result['num_rows'] > 0) {

			$i = 0;

			// Loop through array to create data array
			foreach($records as $rec) {

				$i++;

				// Info

				$this->data[$i]['id'] = $rec['id'];
				$this->data[$i]['headline'] = $rec['headline'];
				$this->data[$i]['date_start'] = $rec['pub_start'];
				$this->data[$i]['date_end'] = $rec['pub_end'];
				$this->data[$i]['teaser'] = $rec['teaser'];

				// Content link

				$this->data[$i]['link'] = $this->content_link."/".$rec['id'];

				// If article has a photo, store info about it in Photo object. 
				// ->setPhotoID() will initiate a new array within object that stores photo info 

				if (isset($rec['photo_id'])) {
					$this->photo->setPhotoID($rec['photo_id']);
					$this->photo->setFolder($rec['folder']);
					$this->photo->setFilename($rec['filename']);
					$this->photo->setW($rec['dim_x']);
					$this->photo->setH($rec['dim_y']);
					$this->photo->setThumbFilename($rec['thumbname']);
					$this->photo->setThumbW($rec['thumb_x']);
					$this->photo->setThumbH($rec['thumb_y']);
					$this->photo->setCaption($rec['caption']);
					$this->photo->setPhotog($rec['photog']);
					$this->photo->setContentID($rec['id']);
				}
			}
		}

		// Load all information about News list into $field array to pass back to  
		// calling script where it will be rendered with a Smarty template. 

		$fields = array();

		$fields['page_title'] = $this->page_title;
		$fields['see_all_label'] = $this->see_all_label;
		$fields['see_all_href'] = $this->see_all_href;
		$fields['data'] = $this->data;
		$fields['pager'] = $this->pager->retrieve();
		$fields['photo'] = $this->photo; 

		return $fields;
	}
}

I basically use this object to pass back an array full of information needed to render a Smarty template and display a list of news articles with thumbnails.

@ahundiak: Thanks for the sample code. Can you explain what you mean by “$pager and $photo seems more like results”? Now that you can see the class above, am I implementing this efficiently in an OOP way?

Thanks!

It’s good to see your class because it makes it clear where the problems are and as I suspected there seem to be too many responsibilities in it. You call this class NewsList, which indicates its purpose is to get a list of news or perhaps manage the list in some other ways but then you say:

so then it turns out the object does not only fetch a news list but also other things that constitute “full information for a Smarty template”. If that is the case then you should name your class NewsTemplateData or something like this.

But let’s get back to NewsList and what the main and only method returns:

$fields['page_title'] = $this->page_title;
$fields['see_all_label'] = $this->see_all_label;
$fields['see_all_href'] = $this->see_all_href;
$fields['data'] = $this->data;
$fields['pager'] = $this->pager->retrieve();
$fields['photo'] = $this->photo;

To me data is the only key that actually is what I would call a news list - and possibly photo if we consider photos as part of the news items. Page title, label and href are data unrelated to the news list and the NewsList class shouldn’t be responsible for that. The NewsList class shouldn’t know or care that you want to display the news list in a Smarty template and neither should it care about the template’s title, label and other information. What if later on you want to get the news list for RSS feeds or consolidate the list into a spreadsheet file or even display it on another page with a different title and no labels, hrefs, etc.? Sure, you could use the same class but then it’s messy because the class does a bunch of things which are unnecessary and to avoid redundant code execution you would need to create a separate class. Reusability suffers.

The same thing is with pager - even though pagination is related somehow to the list I would consider it a separate responsibility because depending on context you may or may not want any pagination, or you may want pagination of different kind, etc. That’s why I would move pagination to a separate class.

Then the usage for your web page would be roughly like this:

$newsList = new NewsList($db, $site);

$pager = new NewsListPager($_GET['page'], $newsList->countAllItems());
$newsItems = $newsList->retrieve($pager->startingItemIndex(), $pager->limitOnPage());

And then you would pass $pager and $newsItems to your template. Your template then would foreach over newsItems to display the news list and fetch all necessary pagination info from the pager object to display page navigation. Of course, the pager should have all necessary methods to provide information like getPageLinks(), getTotalPages(), etc.

You can now see how the NewsList class requires much fewer dependencies because it is dealing with one responsibility only. To your retrieve() method you provide information what range of items to display and the method would create appropriate LIMIT clause for the database query. In a more sophisticated case you could also pass search criteria, etc. in the same way.

There would be no need for a separate photo object because to me photos are logically part of news items, if each news item has one photo then simply let the retrieve method add a photo element to the array that contains the photo url, dimensions, etc. A photo is part of an item’s data the same way as headline, date_start, teaser, etc.

This is just a general idea how to separate your class into two in order to avoid mixing concerns. As always there are many ways to skin a cat :slight_smile:

There are two more issues in your code I want to mention:

Problem 1: You should avoid using new in your classes:

$this->query = new Query;

The new keyword is like a global variable and it results in hidden dependencies that are not visible nor obvious from the outside. Technically, you should also pass the query object in the constructor. However, to me this looks like a reverse case of the previous problem and I would not separate the query object from your database object so much because this means you would need to pass two objects, db and query, to all classes that query the database. I’m sure you don’t ever need to build an SQL query without sending it to a database so the query builder does not need to be passed separately. Instead, you can add a factory to the db class to create the query object on demand. And usage in your class would be like this:

$query = $this->db->getQueryBuilder();
$query->table("news");
$query->select("n.id, n.headline, n.teaser, n.pub_start, n.pub_end, n.url");
$query->from("news n");
$query->where("n.site_id = ".$this->site->id);
$query = $query->getQuery();
$records = $this->db->select($query);

And now you just need to require db as a dependency of your class. Another benefit is that you can easily create as many query objects as you like whereas in your code you were limited to only one query (unless you implemented some other workarounds).

Problem 2:

// If article has a photo, store info about it in Photo object. 
// ->setPhotoID() will initiate a new array within object that stores photo info 

if (isset($rec['photo_id'])) {
    $this->photo->setPhotoID($rec['photo_id']);
    $this->photo->setFolder($rec['folder']);
    $this->photo->setFilename($rec['filename']);
    $this->photo->setW($rec['dim_x']);
    $this->photo->setH($rec['dim_y']);
    $this->photo->setThumbFilename($rec['thumbname']);
    $this->photo->setThumbW($rec['thumb_x']);
    $this->photo->setThumbH($rec['thumb_y']);
    $this->photo->setCaption($rec['caption']);
    $this->photo->setPhotog($rec['photog']);
    $this->photo->setContentID($rec['id']);
}

This kind of code is very misleading and if I didn’t read the comment “setPhotoID() will initiate a new array within object that stores photo info” I would have thought the code was doing something completely different. It’s a hidden surprise that setPhotoID() creates a new array. It’s also a hidden surprise that object Photo holds more than one photo! If it holds more than one photo then name it Photos or PhotoList, etc. And when you add a photo to a PhotoList object then first create the photo array (or object) and then add it with a method like $photoList->addPhoto($photo) - and then all becomes clear without the need for commenting code! Obviously, this is not relevant here any more because we got rid of the strange Photo object (at least in this form) but something to keep in mind in general.

2 Likes

Thanks so much for the detailed explanation and advice.

There are some other methods in NewsList that allow you to set certain parameters for retrieving the news list that I left out, but the retrieve() method is the most important one.

I agree with this, and have been thinking about scrapping the Photo object and just including the photo information as an element of the data array, which is actually how I had it originally set up. My reason for setting up a Photo class was so that I could easily group all photo-related functions in there, available for each photo. But I can also just as easily store and call these functions from a function library, which is basically how it’s done now.

I like this idea a lot. It’s true that the DB class will almost always be used with the Query builder class, so this makes sense. So is this basically what I do in the DB class?

public function getQueryBuilder() 
{
	return new Query; 
}

If so, doesn’t this violate the “avoid using new in your classes” rule?

I suppose the NewsList class has multiple responsibilities and requires several dependencies because I want to keep the implementation simple and to just a few lines of code, and want the NewsList class to do most of the work for me. But I also understand the object of OOP is to break things down into discreet, singular tasks that can be re-used and strung together.

But if I break that class down into smaller pieces, it starts to get a bit complicated to code everything to get all the data the template needs to produce a news list. So would a NewsList factory that essentially does the above be a good way to achieve that? Or is that just adding an unnecessary extra layer of complexity?

In OOP this could be a class like PhotoManager, which could manipulate Photo objects, and eventually real photos. But it’s also important not to go too far and not put all stuff related to photos you can think of into one class because you will again create too many responsibilities and a large, probably messy, class. For example, things like resizing a photo, creating a thumbnail, getting its size, croping, etc. could go into one class but things like sending photos via email, deleting photos from the database - I’d put into separate classes.

Yes, however I should have named it createQueryBuilder() to make it clearer that a new instance is returned every time.

Yes and no. You may guess that this rule cannot be absolute and followed everywhere because you wouldn’t be able to create any object at all. This rule mostly applies to your application classes, those you write to actually do some business logic in your application and delegate object creation to factories. Then your application classes require objects via dependencies instead of creating them, which increases flexibility because you do not have to pass the same object every time - as a dependency you can pass any other object that has the same interface (public methods, etc.) but can work differently under the hood. The new keyword ties you only to one class and one implementation.

So you can treat createQueryBuilder() as a factory baked into the Db class. For me this is fine for this use case but of course it’s also possible to avoid using new if for some reason you wanted more flexibility - then you could delegate Query creation to a separate factory. For this your Db class should require QueryFactory as a dependency in the constructor and you would have this:

public function createQueryBuilder() {
  return $this->queryFactory->create();
}

but honestly to me this would be overkill unless I’d have some really good reason.

You are right but this gets us back to your original problem of passing too many dependencies. Dependency Injection goes well with the Single responsibility principle. If you use Dependency Injection but put multiple dependencies into a class then eventually you will end up with the problem of too many dependencies and then Dependency Injection grows messy. If you use both then this problem would almost never come up.

Yes, the are pros and cons to everything. For simple systems breaking down everything into separate pieces may be more work than it’s worth but as a system grows the benefits become more and more apparent. But even when you are building a simple system and are not stressed for time it’s good to try out new OO styles just to get some practice and see how it can all work together - this way you will prepare yourself for new, potentially larger projects in the future.

You can use a NewsList factory but it will not cause any serious change - a factory is just instantiating an object. Usually, you would need the NewsList object in the controller, so the controller would require the NewsList in its constructor. So the code that instantiates the Controller (the Controller Factory) would create the NewsList object because the Controller requires it.

Basically, the idea is that you have separate objects with single responsibilities that get information needed for a page but these objects do not know they provide information for that page. Another object has the responsibility for calling those other objects, getting information from them and passing them to the template - this is usually done either by a controller or view - the most important is this is done from the outside. And with time you may notice that you can use the same NewsList class for other things than just displaying the news page, or you can use your pagination class not only for the news list but for all kinds of other data, etc. and the pieces become reusable.

Of course, there’s always the trade-off of putting a bit more work up front but it’s worth testing those territories :smiley:

1 Like

This topic was automatically closed 91 days after the last reply. New replies are no longer allowed.