Make a class or not

[fastsol]

So I have this CMS that I have spent the last 5 years building and rebuilding and updating blah blah.  It's a fairly extensive piece work and I am now looking at converting the entire system over to OOP.  I am fairly new to OOP but understand the easy basics of it.  I do fight with the concept of when to make a class and when to just make a normal function.  I have many functions that I built specific to how my system works and some I suppose could be grouped into categories but then many others are just plain functions that only serve one purpose and may only be a few lines long.

 

I will be using the spl_autoload_register() to load the classes as needed which is obviously much easier than including all the function pages that are needed.  So for that reason I like the idea of making classes for everything but it just seems like more than is needed.  So what I am looking for is some insight as to when and how to decide if a class should be made and whether to group functions by category to limit the amount of classes OR to just leave them as normal functions.

 

Here is a example function for creating a thumbnail.  I would guess that I could make a class called Thumb and put this in it, but what exactly would be the benefit of that based on the syntax of the function AND could you show me how you would convert this to OOP to make it worth while putting it in a class? 

// Creates a thumbnail
function createThumb($img, $type, $dest = NULL, $xy = NULL)
{
	if($type=="admin_product")
	{
		$thumb_width = $GLOBALS['admin_thumb_width'];
		$thumb_height = $GLOBALS['admin_thumb_height'];
	}
	elseif($type=="custom")
	{
		
		$thumb_width = ($dest !== NULL) ? $xy['x'] : (int)$_GET['width'];
		$thumb_height = ($dest !== NULL) ? $xy['y'] : (int)$_GET['height'];
	}
	
	$src_size = getimagesize($img);
	
	if($src_size['mime'] === 'image/jpg')
	{$src = imagecreatefromjpeg($img);}
	elseif($src_size['mime'] === 'image/jpeg')
	{$src = imagecreatefromjpeg($img);}
	elseif($src_size['mime'] === 'image/pjpeg')
	{$src = imagecreatefromjpeg($img);}
	elseif($src_size['mime'] === 'image/png')
	{$src = imagecreatefrompng($img);}
	elseif($src_size['mime'] === 'image/gif')
	{$src = imagecreatefromgif($img);}
			
	$src_aspect = round(($src_size[0] / $src_size[1]), 1);
	$thumb_aspect = round(($thumb_width / $thumb_height), 1);
		
	if($src_aspect < $thumb_aspect)//Higher
	{ 
		$new_size = array($thumb_width, ($thumb_width / $src_size[0]) * $src_size[1]);
		$src_pos = array(0, (($new_size[1] - $thumb_height) * ($src_size[1] / $new_size[1])) / 2);
	}
	elseif($src_aspect > $thumb_aspect)//Wider
	{ 
		$new_size = array(($thumb_height / $src_size[1]) * $src_size[0], $thumb_height);
		$src_pos = array((($new_size[0] - $thumb_width) * ($src_size[0] / $new_size[0])) / 2, 0);
	}
	else//Same Shape
	{ 
		$new_size = array($thumb_width, $thumb_height);
		$src_pos = array(0, 0);
	}
			
	if($new_size[0] < 1){$new_size[0] = 1;}
	if($new_size[1] < 1){$new_size[1] = 1;}
	
	$thumb = imagecreatetruecolor($new_size[0], $new_size[1]);
	imagealphablending($thumb, false);
	imagesavealpha($thumb, true);
	imagecopyresampled($thumb, $src, 0, 0, 0, 0, $new_size[0], $new_size[1], $src_size[0], $src_size[1]);
	//$src_pos[0], $src_pos[1] 3rd and 4th of zeros position on above line
	
	if($src_size['mime'] === 'image/jpg')
	{
		if($dest !== NULL)
		{ imagejpeg($thumb, $dest, 95); }
		else
		{
			header('Content-Type: image/jpeg');
			imagejpeg($thumb, NULL, 95);
		}
	}
	elseif($src_size['mime'] === 'image/jpeg')
	{
		if($dest !== NULL)
		{ imagejpeg($thumb, $dest, 95); }
		else
		{
			header('Content-Type: image/jpeg');
			imagejpeg($thumb, NULL, 95);
		}
	}
	elseif($src_size['mime'] === 'image/pjpeg')
	{
		if($dest !== NULL)
		{ imagejpeg($thumb, $dest, 95); }
		else
		{
			header('Content-Type: image/jpeg');
			imagejpeg($thumb, NULL, 95);
		}
	}
	elseif($src_size['mime'] === 'image/png')
	{
		if($dest !== NULL)
		{ imagepng($thumb, $dest); }
		else
		{
			header('Content-Type: image/png');
			imagepng($thumb);
		}
	}
	elseif($src_size['mime'] === 'image/gif')
	{
		if($dest !== NULL)
		{ imagegif($thumb, $dest); }
		else
		{
			header('Content-Type: image/gif');
			imagegif($thumb);
		}
	}
	
	imagedestroy($src);
}

Your insight is appreciated.

[possien]

Generally I group related functions together in classes, for example you could create "class img_functions{...}" for your related functions to images.  The same for db, send mail, time formats, etc. This allows you to create functions within the classes that actually help you use other functions within the class.  Extending classes and other OOP related advantages.  You can cut the number of includes by autoloading classes.  In general you actually cut down the work required to generate pages.   There are also name spaces that are useful if your code is incorporated in other php apps.  You already have a good start with the use of functions.  Check out the PHP manual on classes to give you an idea on what can be expanded and made easier and more helpful.

[ignace]

What possien proposes is not OOP but procedural programming masked as 'OOP' by abusing the class keyword.

 

Since you will be working with images you will have an object Image that you want to support an operation resize(). Since resizing an Image constitutes a new image, your code should act accordingly:

 

class Image .. {
  public function resize(..) {
    return new Image(..);
  }
}

Now for the actual resizing of the Image we can identify several methods:

 

1) A fixed width/height combo

2) Only a width to widen the image

3) Only a height to heighten the image size

4) Increase the image size by a multiplier

5) A ratio for scaling

 

Obviously we will be foolish to force all of this on one method, and we will probably require this on other occassions. A new object is born.

 

class Box {
  public function __construct($width, $height) {}
  public function widen(..) {}
  public function heighten(..) {}
  public function increase(..) {}
  public function scale(..) {}
}

Now add this to our Image::resize() method:

 

class Image .. {
  public function resize(Box $box) {
    // do the image resizing using $box->getWidth(), $box->getHeight()
    return new Image(..); // done!
  }
}

The API in action:

 

//client code
$image = new Image(..);
$thumb = $image->resize($image->getSize()->scale(0.5));
header(..);
print $thumb;

Obviously there is more to OOP then I can tell you in one post. It is also for example possible that one of your classes support an operation that is too big for one class to handle or is not part of it's responsibility (like the above resizing methods). In this case you create multiple classes yet these classes are not known to the 'client' (your controllers that access the public interface of your class) and are never exposed. Which improves maintainability and re-usability.

 

For example you could have a TwitterGateway class that reads your twitter feeds. To do this it requires an OAuth handle. The OAuth handle is a separate class that is never exposed through the public interface of the TwitterGateway class to your client (ie getOauthHandle()), yet it uses this to achieve it's goal (read your feed).

 

The OAuth handle class can be used by other classes though, to access oauth protected resources, thus the OAuth handle class provides re-usable behavior not specific to Twitter.

 

If you still have questions, just ask them.

Edited May 5, 2014 by ignace

[possien]

I agree with everything after "proposing procedural masked by OOP", as mentioned there is a great deal to OOP and you have to start somewhere.  I am sure in his CMS there would be several integrated classes and your description of the image class is spot on.  You  are correct in saying there is a great deal to OOP but the main point I was trying to make was group self-sustainable objects and as you indicated, in a reusable fashion. 

[fastsol]

Thank you for the advice, please keep it coming!!

One thing I came across that I don't understand is in this class below for example.  I wanted to have the make() static so I could call it like Token::make().  The problem is I had to make all the vars in the class static and even in the private functions that are not static I couldn't use $this->, it had to be self:: for everything.  I am lost as to why this is for the not static functions.  Plus when calling Token::make() the __construct doesn't fire either so the vars are not being set properly, why is that.  I can get the vars to set if I put the code from the construct in the make() but I don't think that is the right way to do this.

 

Could you please show me what I am doing wrong.  I usually don't ask for code samples cause I am pretty good at figuring things out and research but this has me lost and I am not really sure how to search for it.

<?php
/*////////////// CORE FILE DO NOT MODIFY ////////////////////*/
class Token
{
	// Sets a session form token
	// Used to prevent CSRF attacks.
	private static $_token_time = "+10 minutes";
	private static $_now;
	private static $_code_length = 10;
	static $_hash;
	static $_set_time;
	static $_rand_code;
	
	public function __construct()
	{
		self::$_rand_code = new RandomCode();
		self::$_hash = new HashInfo;
		self::$_now = time();
		self::$_set_time = strtotime(self::$_token_time, self::$_now); 
	}
	
	private function setValues()
	{
		Session::put('form_token', self::hashed());
		Session::put('form_token_time', self::$_set_time);
	}
	
	private function hashed()
	{ return self::$_hash->create(self::$_rand_code->make(self::$_code_length), self::$_now); }
	
	public static function make()
	{					
		if(Session::exists('form_token') === TRUE && Session::get('form_token_time') < self::$_now)
		{ self::setValues(); }
		else{ self::setValues(); }
		
		return Session::get('form_token');
	}	
}

And yes I know there are other classes being declared in this class, I am not worried about those as I will learn and modify them as I convert all this over

[Jacques1]

This class doesn't make a lot of sense in its current form. It pretends to be a generic class for generating security tokens, but then at the same time it's strictly bound to the session (even a particular session implementation).

 

I think your confusion regarding static methods reflects this underlying design issue.

 

Either turn this into a generic class which simply generates a security token and isn't limited to any particular use case. In this case you probably won't need an object at all, because a function will do the job just fine. Or put all those functionalities into the Session class.

[fastsol]

Yes that makes sense Jacques1. I think I will just add it to the session class. Anyone have comments on the questions I had about the token class i posted?

[Jacques1]

Well, if you make everything static and never instantiate the class, when do you expect the constructor to be called? The whole purpose of the constructor is to initialize a newly created object. No object, no constructor.

 

Apart from that, a class which only consists of static members is a typical case of procedural code masquerading as OOP. 

[fastsol]

Ok ok, so then this might actually be a pointless class?  I am trying to learn the right way to do this but struggling with the whole concept of classes in general.  I have done everything previous in procedural and am very comfortable and good at it in my mind.  I guess the one area that I feel I really like OOP is for database stuff, that seems to be a case that it really shines and can make things easier.  So maybe I shouldn't convert the entire cms to OOP and just incorpoerate certain critical classes.

class Session
{
        // Sets a session form token
	// Used to prevent CSRF attacks.
	private static $_token_time = "+10 minutes";
	private static $_now;
	private static $_code_length = 10;
	static $_hash;
	static $_set_time;
	static $_rand_code;

	public static function put($name, $value)
	{ return $_SESSION[$name] = $value; }
	
	public static function exists($name)
	{ return (isset($_SESSION[$name])) ? TRUE : FALSE; }
	
	public static function get($name)
	{ return $_SESSION[$name]; }
	
	public function __construct()
	{
		self::$_rand_code = new RandomCode();
		self::$_hash = new HashInfo;
		self::$_now = time();
		self::$_set_time = strtotime(self::$_token_time, self::$_now); 
	}
	
	private function setValues()
	{
		Session::put('form_token', self::hashed());
		Session::put('form_token_time', self::$_set_time);
	}
	
	private function hashed()
	{ return self::$_hash->create(self::$_rand_code->make(self::$_code_length), self::$_now); }
	
	public static function make()
	{					
		if(Session::exists('form_token') === TRUE && Session::get('form_token_time') < self::$_now)
		{ self::setValues(); }
		else{ self::setValues(); }
		
		return Session::get('form_token');
	}
}

[Jacques1]

I think the problem is that you're missing the whole point of OOP: It's about objects (hence the name).

 

You don't seem to care about objects at all. All you do is (mis)use classes as a way to group functions and variables. That's not OOP. It's modular programming, a variant of procedural programming.

 

I have the feeling that you're using OOP for the wrong reasons. Maybe someone has told you that procedural code is outdated/bad/uncool/whatever, and now you're trying to make everything look like OOP. That doesn't make much sense. If you have good procedural code, by all means, keep it. It's not like adding a bunch of classes would somehow magically improve code quality. To the contrary.

 

Yes, OOP can be useful in some cases. But you should treat it as a tool to achieve a specific goal, not a fetish.

 

Regarding your session class, I don't see that goal. This again is just a collection of functions. There are no objects with their own state, just a class. So why not have real functions?

[ignace]

Everything Jacques1 said is spot on.

 

@OP What I see from your classes is that you think far too narrow for OOP. You create one class to solve multiple needs at once. Instead you should figure out who the actors are on the stage, define them and then use composition to solve your need.

[gizmola]

I think you have gotten a lot of good advice already.

 

I just wanted to throw in a point about the state of php and architecture.

 

PHP is now at a point where the state of the art is that people are building component libraries that can be mixed an matched in any project easily, with the use of the composer tool.

 

The individual components are of higher quality especially when these components are backed by a high degree of unit test code coverage.

 

It sounds like you are in the process of taking your old world library and approaching a substantial rewrite only to come up with a monolithic customized framework that will be obsolete and out of step with the state of the art in the php world, the day you get it running.

 

If you were to break what you had down into logical components, what would you actually have? Where is the value in your code, and are you sure that there aren't many better components in existence that do everything your code does, only in a far more resilient and unit tested manner?

 

For example, I would highly recommend you take some time looking at this page: http://symfony.com/components

 

Dig into some of these components and compare them to the code you already have. How much overlap is there? Where are the things unique to your CMS you can isolate?

 

If you can turn those elements into unique components, while at the same time replacing the ones you have that were probably done better in the symfony components, then you could approach this exercise entirely differently, and put together a component based version of your cms that leverages untold person - years of quality tested code. That's a very important gut check for you to do right now, in my opinion.

 

This is the type of thinking the Drupal community did a few years back, and the phpbb people and the guy who created the Laravel framework. Component based projects are the present and future of php, especially for projects like yours. If what you find is that you basically need a framework that already handles 90% of things, you might find you're better off just using symfony2 full stack framework with the Doctrine2 ORM and moving all your database design into that, and isolating the specifics of your db designs into the ORM classes.

Edited May 7, 2014 by ignace

[ignace]

Now that we are talking about Symfony. Symfony has it's own CMS of some sort called CMF (if you click on one of the slides, use the arrow keys).

 

It incorporates the ideas outlined in Decoupling Content Management.

 

 

One of the key blocking factors that I've seen with other developers adapting to Symfony is Doctrine and the fact that they still think in terms of database tables and columns. Anyone having done some OOP and written some mapping classes can tell you all about the Object-Relational Impedance Mismatch and the problems associated with it.

 

Thus as to better yourself in OOP you need to remove the idea of a database and think in objects, how they will interact and what messages (the type of data they will return) they will send.

 

You know you have a good OOP model when you have to apply the techniques outlined here.

 

People change and so do requirements. A field you did not have to search on at first is suddenly critical for sprint#n. Thus always keep in mind that change is inevitable and your OOP model will change. Keep everything therefor as simple as possible as to make these changes easy and refactor code pieces that are slowly building a complexity momentum.

Edited May 7, 2014 by ignace