Mixing Procedural code with OOP code — A PHP Conundrum

oop

#21

Okay, I read those pages and also just got your PHP Ninja book. A lot to absorb! I have another question.

I also use a function in a bunch of these classes that simply sets up a few query lines I need in order to load photos related to the content from a photos database. It looks like this:

function photoQuery($photoid, $id_field, Query $query=null)
{
	$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";

	$from = "LEFT JOIN photo ON photo.item_id = $id_field AND photo.type_id = $photoid";

	if ($query) {
		// If Query object passed in, set query params in there
		$query->setSelect($select);
		$query->setFrom($from);
	}

	// Return array with SELECT and FROM statements. Only needed if no Query object passed.
	return array('select' => $select, 'from' => $from);
}

Would this be better if I created a Photo.php class that has this method included? If I did that, it would of course mean I’d have add another dependency to the ClientNews class.

$news = new ClientNews($db, $site, $widget, $client, $photo); 

Whew! Unfortunately, I need a lot of little bits from each of those objects passed in to build the query. But to me this seems so much less efficient and more cumbersome than having a simple global function library. Yes, I’ve been doing procedural programming for a LOOONG time. :slight_smile:


#22

Without seeing the entire class it’s difficult to say, but you’re probably better off starting from the API (how you think the objects should look to someone using them) than from the functions you have. I’m not sure what site or widget are for but consider something like this:

foreach ($client->getNews() as $news) {
	echo $news->title;

	echo $news->content;

	foreach ($news->getPhotos() as $photo) {
		echo '<img src="' . $photo->filename ' />';
	}
}

Working this way around will give you a clearer picture of what objects you need and what they should be doing.


#23

Hi Tom. Okay, here’s the ClientNews class with more detail:

class ClientNews 
{
	protected $db;
	protected $site;
	protected $widget;
	protected $query;
	protected $client_id = 0;

	public function __construct(DB $db, Site $site, Widget $widget=null)
	{
		$this->db = $db;
		$this->query = $db->newQuery();  // Loads a query-builder object 
		$this->site = $site;
		$this->widget = $widget;
	}

	public function setClientId($client_id)
	{
		if (!is_numeric($client_id)) {
			// If alpha ID passed, look up numeric ID for client
			$result = $this->db->select("SELECT id FROM clients WHERE client.alphaid = '$client_id' LIMIT 1");
			$this->client_id = $result['id'];
		} else {
			$this->client_id = $client_id;
		}
	}

	public function getNews()
	{
		// Build Query using the $query object created 

		$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 = :siteid");

		if ($this->widget->show_expired == false) {
			// Show upcoming events
			$this->query->where("UNIX_TIMESTAMP(pub_end) >= UNIX_TIMESTAMP(NOW())");
			$this->query->where("UNIX_TIMESTAMP(pub_start) <= UNIX_TIMESTAMP(NOW())");
		}

		$this->query->order('n.pub_start ASC');

		// Add photo fields to the query, this is currently a function in functions.php 
		photoQuery($this->site->photo_type['news'], "n.id", $this->query);

		// Set up argument list for query
		$query_args = array('siteid' => $this->site->id);

		// Get the fully compiled query from the QueryBuilder object 
		$query = $this->query->getQuery();

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

		// Get info about query, things like how many records found, etc. 
		$result = $this->db->queryInfo();

		/**
		 * Build data array
		 */

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

			// Loop through array to create data array

			foreach($records as $rec) {

				$i = $rec['id'];

				// 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'];
				$this->data[$i]['content'] = $rec['content'];

				// Photo

				$this->data[$i]['photo'] = photoArray($rec);

			}
		}

		return $this->data;
	}
}

Essentially, the ClientNews class constructs a query based on various bits of info from these various objects that have already been created and passed in:

$db is the database object that I created from a database class that uses PDO, and is needed to perform the database query.

$site contains all the settings for a client’s website, but mostly I need the ID number for the client’s website for a WHERE statement. But it contains some website settings that may be useful for altering the query for a particular piece of content.

$widget contains client preferences specific to News content on their website. An example would be if the “expire” date should be ignored or not when loading a list of news items, or how many news items they want to load, etc.

I use a $query object that is created via the $db object in order to build a query using various methods whose names mirror their mysql counterparts, such as $query->from(), $query->where(), etc. Then I just issue $query->getQuery() to retrieve the compiled query that I pass on to the database object. $db->newQuery() in the constructor basically calls a factory method within the $db object to create a Query object as they will almost always be used together.

photoQuery() is a function in a global function library that I’m still using so far. I posted the contents of that function above.

photoArray() is another function that simply sets up the array of info about the related news photo. That array contains things like the image name, dimensions, captions, etc.

The result of this object is an array of all the news records for a particular client of a particular website. I’ll pass that array on to a template object that will merge the data to create the final HTML page.

Hope that makes sense! I’d really appreciate any insight on how I can improve this to be more OOP or make it more efficient, especially where I’m still using functions from a global functions library.


#24

I really like this question because I remember a few years ago when OOP was completely alien to me I had the same question and I couldn’t wrap my head around it how I could possibly get rid of all those utility functions that I used to include in every script and see any benefit from it. And interestingly, now I realize I completely stopped using scripts full of utility functions and I don’t miss them at all - even in big projects they are unnecessary - or, I can even say they contribute to mess.

When you have a large file full of utility functions that you use all over the place you don’t get a good separation of logic and with time it becomes difficult to track where each function is used, what for, etc. When you have a class you can keep it short and dedicated to one functionality and it is completely separate from the rest of your code - because you create an instance of the class, an object, which is a sort of sandboxed piece of code.

Another thing is dependency hell. When you have a large project you would need a lot of functions, many of which would call other functions, which would call other functions, etc. It becomes difficult if you have a large piece of code dedicated to some purpose and you don’t immediately know what other functions you need in order for this code to work - because it can call many functions from your utility file. With OOP you use Dependency Injection, where you specify for each class what other dependencies (objects) are necessary for this class to function - the code becomes clear. And when you want to move that functionality to a different project you simply move the class and its dependencies. And you can even switch the dependencies for other ones, which allows for a kind of flexibility not possible with plain functions.

There are many other benefits of OOP, @TomB’s web site is a very good resource.

Yes, OOP takes more time to write and is usually less efficient but in the long term allows to keep the codebase in order.

For example, in the case of cleanData() I suppose you use the function to parse data from user input. In this case I wouldn’t use such a function at all but instead I’d have a class called formReader that would be dedicated to reading data from a form and changing all input string fields into PHP data in their proper types. Therefore, the calling code would simply use this class to read and parse form data and not even care about how the data is parsed - therefore no need for a cleanData() function. cleanData might then be a private (hidden) method in the formReader class that would be internally used by the public (visible) method that would be called from the outside.


#25

Hi there! Nice to see you again. You were very helpful in two other threads I started about OOP here last year.

I have been trying to wrap my head around OOP principles for a few years now, and boy it has been a tough slog! I’m beginning to think rocket science would be easier. :grimacing:

Okay, so, in the case of the sample class I posted above, it seems like I’d need to create at least 2-3 more classes that need to be passed to the ClientNews object to make it operate. My constructor would end up looking something like this:

public function __construct(DB $db, Site $site, Photos $photo, FormReader $form, Widget $widget=null)

Is that an unreasonable number of dependencies for an object that simply does a database query and sends back an array of results? Unfortunately, I need quite a few bits of info from various places for a single query.

I much better understand dependency injection now, but dependency injection “containers” are still a bit of a mystery to me. It seems like they add even more complexity to OOP code (and more code in general), and obscure what’s really going on anyway. When I want to know what scripts are using certain functions from my function library, I simply do a search for the function name in the folder that contains all my scripts. That seems pretty easy to me. :slight_smile:

I’m also reading @TomB’s book “PHP Novice to Ninja” right now hoping it will shed some light on a few of these OOP concepts I just can’t wrap my head around right now.


#26

That may be true but this is actually one of the advantages of OOP - you create small classes and each one is designed to do only one task. Then another class may use a few of those small classes (required as dependencies) and carry out a bigger task by utilizing those classes. So in principle you are right but I suspect the separation should be slightly different than you thought…

No, you certainly don’t need so many dependencies for a class that does a database query and sends back results. But your ClientNews class does more than that but your probably don’t see it - it fetches data from Site, Photos, FormReader and Widget objects - maybe this is subtle in this example but to me this counts as additional tasks.

An example of how you might split it into more independent units might be first to create a class that really only fetches the results for client news and nothing more (I will only show method definitions without code for simplicity):

class ClientNewsReader {

  private $db;

  public function __construct($db) {
    $this->db = $db;
  }

  public function getNews($site_id, $client_id = null) {
    // db queries, etc
    // [...]
    return $news;
  }
}

This is what you might call a class with only one task. Then your ClientNews class would use this class instead of Db:

class ClientNews {
  private $clientNewsReader;

  public function __construct(ClientNewsReader $clientNewsReader) {
    $this->clientNewsReader = $clientNewsReader;
  }

  public function getNews($site_id, $client_id = null) {
    return $this->clientNewsReader->getNews($site_id, $client_id);
  }
}

And now your list of dependencies in the constructor is smaller.

Treat it only as an example of how a class might be split into two with more specialized purposes. In this case I’m not sure if such separation is really needed if what you presented in your ClientNews class is all that there is to it. I admit that in such very simple cases I don’t try to go too far with perfect OO separation of concerns but I use this idea whenever I expect I may include too diverse responsibilities in a single class.

In your case I would only make the dependencies more specific - for example, if you only need site_id then there is no need to require the whole Site object - it’s enough to require site_id and then the class is simpler and more independent because it doesn’t even have to know about the existence of Site. It looks like you need site_id as search criteria so that’s why I would require it as an argument to getNews() method instead of the constructor - in that case one instance of the class can be used to fetch news for different site_id`s.

The only thing that doesn’t fit in here to me is FormReader - reading data from a form is a distinct task from fetching news from the database and should be done elsewhere - not in the same class that you are fetching data from the database. If you need data from a form then in the calling code I’d use FormReader to read data from form and then pass the required data to ClientNews object so that ClientNews does not deal with reading the form. Or, I would create a class like ClientNewsFormSearch that would deal with fetching data from a form and based on the data would fetch news - then this class would read the data from the form and then use the (required) ClientNewsReader (like I presented above) to read the data from the database.


#28

I just saw your reply, for some reason I didn’t get a notice that you replied to this topic from the forum software.

Thanks for your detailed and helpful response. I’m a bit confused, though, how I go about implementing the two classes you created. Is this how I’d do it?

$newsreader = new ClientNewsReader($db, $widget);

$clientnews = new ClientNews($newsreader);

$news = $clientnews->getNews(999,888);

If so, I don’t get what the purpose of creating two classes does. Why not do this instead…

$newsreader = new ClientNewsReader($db, $widget);

$news = $newsreader->getNews(999,888);

That seems more streamlined and more obvious to me. But that’s because after two years of trying to learn OOP, I don’t think I’m getting any closer to understanding OOP. The more I learn, the more confused I become. :slightly_frowning_face:


#29

Yes, you got it right!

The reason is separation of concerns. But as I said earlier, your example is so simple that I doubt it makes sense to do it in two classes - that’s why I said it should be treated as a theoretical exercise.

But if you want to add reading data from a web from form then it would be a good idea to use another class:

$formReader = new FormReader($_POST);
$cnReader = new ClientNewsReader($db);
$news = $cnReader->getNews($formReader->getField('client_id'), $formReader->getField('site_id'));

In this case FormReader and ClientNewsReader classes are separate and don’t know about each other. They deal with two different responsibilities and the benefit is that you can re-use each of them in different contexts, for example you can use the FormReader to read data from other forms, or you can use ClientNewsReader to get news for a JSON API you built for external sites - and then client_id and site_id will not come from any form but, for example, from API parameters.

Another way to organize the classes:

$cnReader = new ClientNewsReader($db);
$cnfSearch = new ClientNewsFormSearch($cnReader, $_POST);
$news = $cnfSearch->getNews();

Here ClientNewsFormSearch is specialized to deal with fetching news based on search criteria in this specific form so you will not be using it to fetch news from other contexts - but you might use it for other forms which are similar. ClientNewsReader is the same as above - it’s independent from the idea of forms, it can be used in any context to fetch news.