More on dependency Injection

Hi;

Which code is a better design? Is it not a disadvantage to have too many lines of code compared to less code but more coupling?

class A
	{
		private $method;
		public function pay($id)
			{
				$this->method = new B();
				return $this->method->foo($id);
			}
	}

class B
	{
		public function foo($arg)
			{
				return $this;
			}
	}	

$object = new A();
var_dump($object->pay(3400));	
class A
	{
		private $method;
		private $D;
		public function __construct(D $D)
			{
				$this->D = $D;
				$this->method = $this->D;
			}
		public function pay($id)
			{
				return $this->method->foo($id);
			}
	}


class B implements D
	{
		public function foo($arg)
			{
				return $this;
			}
	}

class C implements D
	{
		public function foo($arg)
			{
				return $this;
			}
	}	

interface D
	{
		public function foo($arg);
	}		

$object = new A(new B);
var_dump($object->pay(3400));	

Hi,

I would say that your second example is the better design.

Firstly, it is easier to test. When you’re testing class A, you can pass in a mock object which implements interface D. With your first example there’s no way to test A independently of B.

Secondly, the design is more flexible. If we consider a more concrete example, such as the payment service and payment types in your other thread, the system can be easily extended to include new payment types without having to alter any of the existing classes.

Hi; I like your monkey;

1 thing that concerns me is the way I instantiate these classes. I know you say it is correct design but t just doesn’t look right.

$object = new A(new B(new E()));

Which is 1 way of implementing this:

Yes the code is flexible and not coupled but is it really supposed to look like that? This kind of instantiating can get really messy. I am dealing with a code that has a lot more dependencies. I don’t think its cool to write 5 lines of injection is it?

Here is the whole lot.

<?php 
class A
	{
		private $method;
		
		private $D;

		public function __construct(D $D)
			{
				$this->D = $D;
				$this->method = $this->D;
			}
		public function pay($id)
			{
				return $this->method->foo($id);
			}
	}


class B implements D
	{
		private $dependency;
		public function __construct(E $E)
			{
				$this->dependency = $E;
			}
		public function foo($arg)
			{
				return $this->dependency;
			}
	}

class C implements D
	{
		public function foo($arg)
			{
				return $this;
			}
	}	

interface D 
	{
		public function foo($arg);
	}

class E implements F
	{
		public function __construct()
			{
				return $this;
			}
	}

interface F
	{
		public function __construct();
	}		


$object = new A(new B(new E()));
var_dump($object->pay(3400));	

?>

I checked Pimple as you said,same deal, lots of code. I like injection better really.

$container = new Pimple();
$container['oauth'] = function($c) {
    return new OAuth();
};
$container['db'] = function($c) {
    return new DB();
};
$container['tweet_service'] = function($c) {
    $twService = new TwitterService();
    $twService->setDB($c['db']);
    $twService->setOauth($c['oauth']);
    return $twService;
};
$container['social_feeds'] = function($c) {
    $socialFeeds = new SocialFeeds();
    $socialFeeds->setTwitterService($c['tweet_service']);
    return $socialFeeds;
};
 
$socialFeeds = $container['social_feeds'];
$socialFeeds->getSocialFeeds();

My final goal is to fix this biatch:

<?php
class SendCampaign extends BaseController {
	public function __construct()
		{
			$this->beforeFilter('login_check');	
		}
	public function send($id)
		{
			
			
			//Set data
			$data['name'] = $name = ucfirst(Input::get('name')); 
			$data['email'] = $email = Input::get('email');
			$data['receiver'] = Input::get('email');
			$time_sent = time();
			
			//Validate posted data
			$validator = Validator::make(
				   array('email' =>$email ),
				   array('email' => array('required', 'email'))
				);
			if ($validator->fails())
				{
				   Session::put('campaign_sent', '<p class="text-error">Invalid email address</p>');
				   return Redirect::to('email');
				}
			
			//Load campaign
			$results = Campaign::find($id);
			$view_file = $results->view_file;

			//Attach deals
			
			if(is_array(Input::get('deals')))
				{
					$in = "(";
					foreach(Input::get('deals') as $val)
						{
							$in .= $val.",";
						}
					$in = substr($in, 0, -1);	
					$in .= ")";	
					echo $in;

					$data['deals']="";

				    $deals = DB::select('SELECT  domain_deals.id AS DID,
				    domain_deals.title AS DT,
				    domain_deals.status AS DSTAT,
				    domain_deals.original_value AS OV,
				    domain_deals.end_date AS end_date,
				    domain_deals.deal_value AS DV
				    FROM domain_deals INNER JOIN
				    domain_business ON domain_business.id = domain_deals.business_id
				    WHERE domain_deals.id IN '.$in.' GROUP BY domain_deals.id
											');


					$data['deals'] .= "<table><tr>";
					foreach($deals as $val =>$row)
						{
							$seconds_left = strtotime($row->end_date) - time();
							$days_left = floor($seconds_left/(3600*24));
							if($days_left<0){ $days_left = 0;}
							$discount = round(($row->DV*100)/$row->OV);
							$discount = 100 - $discount;
							$data['deals'] .= "<td>
							
								<table style = \\"float:left; width:350px;\\">
									<tr><td colspan = \\"2\\"><a href = \\"http://www.domain.com.au/details/show/".$row->DID."\\"><img src = 'http://domain.com.au/uploads/".$row->DID.".jpg' width=\\"250px\\" height = \\"250px\\" style = \\"-moz-border-radius: 10px; border-radius: 10px;\\"></a></td></tr>
									<tr><td colspan = \\"2\\"><a style = \\"; text-decoration:none;\\" href = \\"http://www.domain.com.au/details/show/".$row->DID."\\"><strong style = \\"font-family:Arial; font-size:16px; text-decoration:none;\\">".$row->DT."</strong></a></td></tr>
									<tr>
										<td colspan = \\"2\\" style=\\"border-bottom:1px solid #cccccc\\"><br /><strong style = \\"color: #ED2930; margin:0; padding:0;
										    font-family: Arial,Helvetica,sans-serif;
										    font-size: 16px;
										    font-weight: bold;\\">$".$row->DV."
										    </strong>&nbsp;&nbsp;&nbsp;
										    <strong style = \\"color: #202020; margin:0; padding:0; text-decoration:line-through;
										    font-family: Arial,Helvetica,sans-serif;
										    font-size: 16px;
										    font-weight: bold;\\">$".$row->OV."
										    </strong><br /><br />
										</td>
									</tr>
									<tr>
										<td><br />
											<span style = \\"color: #444444; margin:0; padding:0;
										    font-family: Arial,Helvetica,sans-serif;
										    font-size: 14px;\\"><img src = \\"http://domain.com.au/images/clock-image.png\\">&nbsp;".$days_left." days Left
										    </span>&nbsp;&nbsp;&nbsp;
										</td>
										<td align = \\"right;\\">    
										    <span style = \\"color: #444444; 
										    font-family: Arial,Helvetica,sans-serif;
										    font-size: 14px;
										    \\"><img src = \\"http://domain.com.au/images/voucher-image.png\\">&nbsp;".$discount."% discount
										    </span>
									    </td>
									</tr>


									    </table>
										    
									    </td>";
						}	
					$data['deals'] .= "</tr></table>";	
					//exit();
					

					
				}
			else
				{
					$data['deals'] = "";
				}		
											
			//echo $data['deals'];exit();	
			//Load sende'r signature details signature
			$sender_id = Session::get('id');
			$sender_details = Admin::find($sender_id);
			$data['sender_name'] = ucwords($sender_details->name);
			$data['sender_email'] = $sender_details->email;
			$data['position'] = $sender_details->position;
			$data['mobile'] = $sender_details->mobile;
			$data['phone'] = $sender_details->phone; 

			//Check if view exists
			if (!View::exists($view_file))
				{
					 Session::put('campaign_sent', '<p class="text-error">The email template you are trying to send does not exists!</p>');
				   return Redirect::to('email');
				}	
			
			//Send email
			Mail::send($view_file, $data, function($message)
				{
				    $message->to(Input::get('email'), Input::get('name'));
				    $message->from('support@domain.com', 'domain Support');
				    $message->cc('pmdg3@yahoo.com', 'Behnam');
				    $message->subject(Input::get('email_title'));
				});
			
			//Save report   domain_campaign_sent table
			$CampaignReport = new CampaignReport;
			$CampaignReport->name = $name;
			$CampaignReport->email = $email;
			$CampaignReport->campaign_id = $id;
			$CampaignReport->time_sent = $time_sent;
			$CampaignReport->save();

			
			
			



			
			//If member already not in this mailing list 
			$results = DB::select('select * from domain_individual_campaign_signup where email = ?', array($email));
			
			if(count($results)==0)
				{
					//Save new member
					$CampaignSignup = new CampaignSignup;
					$CampaignSignup->name = $name;
					$CampaignSignup->email = $email;
					$CampaignSignup->campaign_id = $id;
					$CampaignSignup->time_sent = $time_sent;
					$CampaignSignup->save();
				}	

			
			//If member already not exists
			$results = DB::select('select * from domain_mailinglist_member where email = ? AND mailing_list_id = ?', array(Input::get('email'), Input::get('mailinglist_id')));
			if(count($results)==0)
				{
					//Sign this user to domain_IC_MLG table //
					$ICSML = new ICSML;
					$ICSML->mailing_list_id = Input::get('mailinglist_id');
					$ICSML->email = Input::get('email');
					$ICSML->save();
				}
			

			//Redirect 
			Session::put('campaign_sent', '<p class="text-error">Campaign sent successfully</p>');
			return Redirect::to('email');
		}	
}

I know Laravel takes care of all that but I like to know the rules rather than just using Lara. Made that mistake with CI.

Thanks

Yep. That is what dependency injection looks like.

This is where dependency injection containers (aka inversion of control containers) come in. It may or may not take a few lines of code to configure a container (some can use type hints to reduce the amount of required code) but basically your application code reduces to:

$object = $container['a']; 

Which is pretty nice.

Laravel has it’s own container: http://laravel.com/docs/ioc

It’s also easy enough to plug in the Symfony 2 container which has some advanced capability: http://symfony.com/doc/current/components/dependency_injection/index.html

Hi guys

Thanks you for your replies.

I have been working very hard on this and finally finished this. Please note my goal is to write simple OOP code without coupling and dependencies ( I think they are the same ). I am not going to use containers yet.

Would you please let me know if I am doing well or not? Don’t be too hard on me :slight_smile:

It appears to me that the main class is just like a centre core that does NOTHING but using other classes ( I think they are called services, right? ) to do the job.

Of course these are going to be separate files and I will use auto loader to load these. Well 1 step at a time :wink:

<?php
class sendCampaign
	{
		private $postedDeals = array();
		private $dealsList = array();
		private $buildEmailView;
		private $signatureClass;
		private $emailContent;
		private $mailingList;
		private $postedList;
		private $validation;
		private $signature;
		private $template;
		private $mailer;
		private $deals;
		private $title;
		private $list;
		
		
		public function __construct($campaignId = NULL, validation $validation, deals $deals, signatureClass $signatureClass, template $template, email $email, mailingList $mailingList, mailer $mailer)
			{
				if(!$this->forcePost())
					return false;
				
				if(!$this->assignPostedData())
					return false;	
				
				if(!$this->validateCampaign($campaignId, $validation))
					return false;
				
				if(!$this->validateDeals($this->postedDeals, $validation))
					return false;
				
				if(!$this->addDealsToTemplate($this->postedDeals, $template))
					return false;

				if(!$this->receiversAddresses($campaignId, $mailingList))
					return false;
				
				if(!$this->sendEmails($this->emailContent, $this->title, $this->list, $mailer))
					return false;
			}

		
		private function sendEmails($content, $title, $receivers, $mailer)
			{
				$this->mailer = $mailer;
				if(!$this->mailer->sendEmail($content, $title, $receivers))
					{
						errors::showError('Emails were NOT sent successfully');
						return false;
					}	
			}

		private function receiversAddresses($campaignId, $mailingList)
			{
				$this->mailingList = $mailingList;
				return $this->list = $this->mailingList->buildList($campaignId);
			}

		private function assignPostedData()
			{
				$this->postedDeals = ($_POST['postedDeals']) ? $_POST['postedDeals'] : $this->postedDeals;
				$this->postedList = ($_POST['postedList']) ? $_POST['postedList'] : $this->postedList;
				$this->title = ($_POST['title']) ? $_POST['title'] : $this->title;
				return true;
			}	

			
		private function forcePost()
			{
				if(!$_POST)
					{
						errors::showError('No data has been posted');
						return false;
					}
				return true;	
			}	
		private function addDealsToTemplate($deals, $template)
			{
				if(!$loadedTemplate = $template->load('dealsList'))
					{
						errors::showError('Could not load email template');
						return false;
					}	
				else
					{
						echo "Template loaded<br />";
						$this->emailContent = $template->addDeals($loadedTemplate, $deals);
						return $this->emailContent;
					}	
			}	
		private function validateCampaign($id, $validation)
			{
				$this->validation=$validation;
				if(!$this->validation->validateCampaignIdDB($id))
					return false;
				return true;		
			}	
		private function validateDeals($deals, $validation)
			{
				$this->validation=$validation;
				if(!$this->validation->validateDealIdDB($deals))
					{
						errors::showError('Posted deals are invalid');
						return false;
					}	
				return true;
			}	

		private function buildSignature($signatureClass, $id)
			{
				$this->signatureClass = $signatureClass;	
				$this->signature = $this->signatureClass->getSignatureDetails($id);
			}	
		private function generateListOfDeals($deals, $buildEmailView)
			{
				$this->deals = $deals;
				if(count($this->dealsList = $this->deals->dealsList(array()))==0)
				   $this->dealsList = "";
				return $this->dealsList;
			}	
	
	}



class mailingList implements mailingListInterface
	{
		public function buildList($campaignId)	
			{
				if(1==1) //check with DB
					{
						$list = array('email@eamil.com', 'email2@email.com');
						return $list;
					}
				else
					{
						return false;
					}	
				
			}
	}

interface mailerInterface
	{
		public function sendEmail($content, $title, $mailingList);
	}	

class mailer implements mailerInterface
	{
		private $headers;
		public function sendEmail($content, $title, $mailingList)	
			{
				foreach($mailingList as $email)
					{
						mail($email, $title, $content, $this->headers);
					}
				echo "Emails were sent successfully";	
				return true;		
			}
	}






interface mailingListInterface
	{
		public function buildList($campaignId);
	}	

class email implements emailClassInterface
	{
		public function sendEmail($content, $mailingList)	
			{
				return true;
			}
	}



class signatureClass implements signatureClassInterface
	{
		public function getSignatureDetails($id)	
			{
				return true;
			}
	}

class errors	
	{
		public static function showError($errorMessage=NULL)
			{
				echo $errorMessage;
			}
	}	

class validation
	{
		public function validateCampaignId($id=NULL)
			{
				if(is_null($id) || !is_numeric($id))
					{
						errors::showError('Invalid Campaign Id<br />');
						return false;
					}	
				else
					{
						echo "Validation passed.<br />";
						return true;			
					}	
			}
		
		public function validateCampaignIdDB($id)	
			{
				if(1!=1)//Check with DB
					{
						echo "Campaign NOT found in DB.<br />";
						return false;
					}	
				echo "Campaign found in DB successfully.<br />";	
				return true;
			}
		public function validateDealIdDB($postedDeals)	
			{
				//Check with DB, if DB results exist
				if(count($postedDeals)>0)
					{
						//Check with DB
						echo "Deals found in DB<br />";
						return true;
					}
				else
					{
						echo "No deals posted<br />";
						return false;
					}	
				
			}	
	}	
class deals implements dealsInferace
	{
		public function dealsList($deals = NULL)	
			{
				return $deals;
			}		
	}

class template
	{
		private $list;
		public function load($file)
			{
				$file="templates/".$file.".php";
				if(file_exists($file))
					{
						if(!file_get_contents($file))
							return false;
						else
							return $file;	
					}
				else
					{
						return false;
					}		
			}
		public function addDeals($file, $deals)
			{
				foreach($deals as $val)
					{
						$this->list .= "<tr><td>".$val."</td></tr>";
					}
				$file = str_replace("#DEALS#", $this->list, file_get_contents($file));
				echo "Deals successfully added to the template<br />";
				return $file;
			}	
	}	

interface dealsInferace
	{
		public function dealsList($deals = NULL);
	}

interface signatureClassInterface
	{
		public function getSignatureDetails($id);
	}	

interface buildEmailViewInterface
	{
		public function dealsList($deals);
	}

interface emailClassInterface
	{
		public function sendEmail($content, $mailingList);	
	}				

$object = new sendCampaign(2, new validation(), new deals(), new signatureClass(), new template(), new email(), new mailingList(), new mailer());

?>

Hi,

I don’t know if you saw my latest reply to your previous thread, but I made some suggestions on how you could refactor your controller action there.

A couple things I noticed at first glance:

  1. Your sendCampaign constructor is doing all the work. It should only be responsible for setting up the object instance, while the object’s public methods actually do the work. E.g. assigning the dependencies that were passed in to properties on the object:

class SendCampaign {

    protected $mailer;

    public function __construct(Mailer $mailer)
    {
        $this->mailer = $mailer;
    }
}
  1. Some of your class methods are directly accessing the $_POST superglobal. Your class shouldn’t know anything about that, as it’s the controller’s responsibility to access the request parameters and pass them to your model layer. Plus, passing params into your class makes it easier to test.

  2. If your classes have too many dependencies, it’s a sign that the class is doing too much, and should be split up into smaller units.

Oh no I must have missed it. I go read on that now. Thanks

I understand what you mean by the constructor being responsible for many things but at the same time I don’t know how else to do it. In my knowledge I should pass the instances of other classes ( I think they are called services, or repositories, yes? no? ) to the “main” class and using the right logic I call the right methods/classes to do each job.

Umm, what about this? Isn’t this what I am doing?

I made a method specifically for this. Isn’t it the right way?

private function assignPostedData()
			{
				$this-&gt;postedDeals = ($_POST['postedDeals']) ? $_POST['postedDeals'] : $this-&gt;postedDeals;
				$this-&gt;postedList = ($_POST['postedList']) ? $_POST['postedList'] : $this-&gt;postedList;
				$this-&gt;title = ($_POST['title']) ? $_POST['title'] : $this-&gt;title;
				return true;
			}	

All of the things this class does are for 1 reason. Since a class “is” a page name which “is” a controller. While we are on this would you please tell me about services and repositories and how I can make it 100% clear what business logic means. Thanks a lot I am gonna read the other thread now.

Thank you

Umm, what about this? Isn’t this what I am doing?
[/quote]

Your constructor is calling other class methods and passing them the dependencies, rather than assigning them to class properties directly.

Take your sendEmails method as an example. We could rewrite it something like this:


class sendCampaign  
{ 
	protected $mailer;

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

	public function sendEmails($content, $title, $receivers) 
	{ 
	    return $this->mailer->sendEmail($content, $title, $receivers); 
	} 
}

// Using the class
$service = new SendCampaign(new Mailer);
$service->sendEmails($view, 'May 2014 Campaign', $recipients);

Note that the constructor doesn’t call any methods that actually do any work, it just sets up the mailer so it’s ready for when we call sendEmails.

No because our class shouldn’t know about the $_POST array, that’s the controller’s job.

Imagine that your project also requires a command line interface at some point, to allow campaigns to be sent out from the terminal. The data would no longer be coming via a HTTP request. If you keep the responsibilities separated, then your service class wouldn’t have to change. That’s the whole point of the SRP - a class should only have one reason to change.

Repositories are classes that deal with storing and retrieving entities. If you have a User repository, all your application needs to know is that’s where to get existing User objects from, and put new ones. The ‘how’ is completely irrelevant to the app. Assuming the repositories implement a standard interface, you could swap out a MysqlUserRepository for an XmlUserRepository and your application shouldn’t notice (or care).

Service classes are a place to put logic that doesn’t belong in your entity classes. An example of this would be having a service class which deals with a 3rd-party API. If your app uses an email service like Mailgun or Mandrill, you would have a service class (or classes) which encapsulate the logic for dealing with this.

Business logic is basically the rules of your application… things like “When a campaign is sent out, add the recipient to the mailing list if they are not already on it”. This is distinct from application logic, for example “if the template file isn’t found, throw an error of type xyz”.

Hi;

I made the changes to constructor as you said. I am sorry but I dont really understand how I should deal with requests ($_POST or from terminal).

Would you please show that as well?

Thanks a lot for helping.

class sendCampaign
	{
		private $postedDeals = array();
		private $dealsList = array();
		private $buildEmailView;
		private $signatureClass;
		private $emailContent;
		private $mailingList;
		private $postedList;
		private $validation;
		private $signature;
		private $template;
		private $mailer;
		private $deals;
		private $title;
		private $list;
		
		
		public function __construct($campaignId = NULL, validation $validation, deals $deals, signatureClass $signatureClass, template $template, email $email, mailingList $mailingList, mailer $mailer)
			{
				$this->mailer = $mailer;
				$this->mailingList = $mailingList;
				$this->validation=$validation;
				$this->signatureClass = $signatureClass;
				$this->deals = $deals;


				if(!$this->forcePost())
					return false;
				
				if(!$this->assignPostedData())
					return false;	
				
				if(!$this->validateCampaign($campaignId, $validation))
					return false;
				
				if(!$this->validateDeals($this->postedDeals, $validation))
					return false;
				
				if(!$this->addDealsToTemplate($this->postedDeals, $template))
					return false;

				if(!$this->receiversAddresses($campaignId, $mailingList))
					return false;
				
				if(!$this->sendEmails($this->emailContent, $this->title, $this->list, $mailer))
					return false;
			}

		
		private function sendEmails($content, $title, $receivers)
			{
				if(!$this->mailer->sendEmail($content, $title, $receivers))
					{
						errors::showError('Emails were NOT sent successfully');
						return false;
					}	
			}

		private function receiversAddresses($campaignId)
			{
				return $this->list = $this->mailingList->buildList($campaignId);
			}

		private function assignPostedData()
			{
				$this->postedDeals = ($_POST['postedDeals']) ? $_POST['postedDeals'] : $this->postedDeals;
				$this->postedList = ($_POST['postedList']) ? $_POST['postedList'] : $this->postedList;
				$this->title = ($_POST['title']) ? $_POST['title'] : $this->title;
				return true;
			}	

			
		private function forcePost()
			{
				if(!$_POST)
					{
						errors::showError('No data has been posted');
						return false;
					}
				return true;	
			}	
		private function addDealsToTemplate($deals, $template)
			{
				if(!$loadedTemplate = $template->load('dealsList'))
					{
						errors::showError('Could not load email template');
						return false;
					}	
				else
					{
						echo "Template loaded<br />";
						$this->emailContent = $template->addDeals($loadedTemplate, $deals);
						return $this->emailContent;
					}	
			}	
		private function validateCampaign($id)
			{
				if(!$this->validation->validateCampaignIdDB($id))
					return false;
				return true;		
			}	
		private function validateDeals($deals)
			{
				if(!$this->validation->validateDealIdDB($deals))
					{
						errors::showError('Posted deals are invalid');
						return false;
					}	
				return true;
			}	

		private function buildSignature($id)
			{
				$this->signature = $this->signatureClass->getSignatureDetails($id);
			}	
		private function generateListOfDeals( $buildEmailView)
			{
				if(count($this->dealsList = $this->deals->dealsList(array()))==0)
				   $this->dealsList = "";
				return $this->dealsList;
			}	
	
	}



class mailingList implements mailingListInterface
	{
		public function buildList($campaignId)	
			{
				if(1==1) //check with DB
					{
						$list = array('email@eamil.com', 'email2@email.com');
						return $list;
					}
				else
					{
						return false;
					}	
				
			}
	}

interface mailerInterface
	{
		public function sendEmail($content, $title, $mailingList);
	}	

class mailer implements mailerInterface
	{
		private $headers;
		public function sendEmail($content, $title, $mailingList)	
			{
				foreach($mailingList as $email)
					{
						mail($email, $title, $content, $this->headers);
					}
				echo "Emails were sent successfully";	
				return true;		
			}
	}






interface mailingListInterface
	{
		public function buildList($campaignId);
	}	

class email implements emailClassInterface
	{
		public function sendEmail($content, $mailingList)	
			{
				return true;
			}
	}



class signatureClass implements signatureClassInterface
	{
		public function getSignatureDetails($id)	
			{
				return true;
			}
	}

class errors	
	{
		public static function showError($errorMessage=NULL)
			{
				echo $errorMessage;
			}
	}	

class validation
	{
		public function validateCampaignId($id=NULL)
			{
				if(is_null($id) || !is_numeric($id))
					{
						errors::showError('Invalid Campaign Id<br />');
						return false;
					}	
				else
					{
						echo "Validation passed.<br />";
						return true;			
					}	
			}
		
		public function validateCampaignIdDB($id)	
			{
				if(1!=1)//Check with DB
					{
						echo "Campaign NOT found in DB.<br />";
						return false;
					}	
				echo "Campaign found in DB successfully.<br />";	
				return true;
			}
		public function validateDealIdDB($postedDeals)	
			{
				//Check with DB, if DB results exist
				if(count($postedDeals)>0)
					{
						//Check with DB
						echo "Deals found in DB<br />";
						return true;
					}
				else
					{
						echo "No deals posted<br />";
						return false;
					}	
				
			}	
	}	
class deals implements dealsInferace
	{
		public function dealsList($deals = NULL)	
			{
				return $deals;
			}		
	}

class template
	{
		private $list;
		public function load($file)
			{
				$file="templates/".$file.".php";
				if(file_exists($file))
					{
						if(!file_get_contents($file))
							return false;
						else
							return $file;	
					}
				else
					{
						return false;
					}		
			}
		public function addDeals($file, $deals)
			{
				foreach($deals as $val)
					{
						$this->list .= "<tr><td>".$val."</td></tr>";
					}
				$file = str_replace("#DEALS#", $this->list, file_get_contents($file));
				echo "Deals successfully added to the template<br />";
				return $file;
			}	
	}	

interface dealsInferace
	{
		public function dealsList($deals = NULL);
	}

interface signatureClassInterface
	{
		public function getSignatureDetails($id);
	}	

interface buildEmailViewInterface
	{
		public function dealsList($deals);
	}

interface emailClassInterface
	{
		public function sendEmail($content, $mailingList);	
	}				

$object = new sendCampaign(2, new validation(), new deals(), new signatureClass(), new template(), new email(), new mailingList(), new mailer());

Basically your controllers are responsible for receiving input from a request, and returning a response (html, json, a file, whatever). This means that classes in your model layer (eg. entities, services) should not be accessing superglobals like $_GET and $_POST, or echoing HTML.

Here’s an example of one way you could rewrite your code to keep the request/response stuff isolated to your controller. It also moves the actioning of the work out of the SendCampaign contructor. It’s based on your code, but I’ve assumed the existence of some new classes.

controller class


class CampaignController {

    public function send($campaignId)
    {
        $dealIds = Input::get('deals');
        $title   = Input::get('title');

        $service = new SendCampaign(new CampaignRepository, new DealRepository, new TemplateLoader, new Mailer);

        try {
            $service->send($campaignId, $dealIds, $title);
        } catch (Exception $e) {
            // Get error msg from exception and do something with it
        }

        // Display a 'success view' / redirect
    }

}

SendCampaign class


class SendCampaign  
{ 
    protected $campaignRepository;
    protected $dealRepository;
    protected $templateLoader;
    protected $mailer; 
     
    public function __construct(CampaignRepository $campaigns, DealRepository $deals, TemplateLoader $loader, Mailer $mailer) 
    { 
        $this->campaignRepository = $campaigns;
        $this->dealRepository = $deals;
        $this->templateLoader = $loader;
        $this->mailer = $mailer; 
    }

    public function send($campaignId, $dealIds, $title)
    {
        $campaign = $this->getCampaign($campaignId);
        $deals = $this->getDeals($dealIds);

        $content = $this->templateLoader->load('dealsList');
        $content->set('deals', $deals);

        $recipients = array();
        foreach ($campaign->recipients as $recipient) {
            $recipients[] = $recipient->email;
        }

        $this->mailer->sendEmail($content, $title, $recipients);
    }

    protected function getCampaign($campaignId)
    {
        $campaign = $this->campaignRepository->getById($campaignId);

        if (!$campaign) {
            throw new CampaignNotFoundException;
        }

        return $campaign;
    }

    protected function getDeals($dealIds)
    {
        $deals = $this->dealRepository->findById($dealIds);

        if (empty($deals)) {
            throw new DealsNotFoundException;
        }

        return $deals;
    }
}

Let me know if you want me to explain anything in more detail.

I just wanted to step in a reiterate what fret is saying. What he is saying is not just for MVC design, but for most everything else. You’ll want to ensure that everything your class / function is using is being passed to it (dependency injection) and not being access through any type of global variable ($_POST, $_GET, $GLOBAL, etc). If you need $_POST data, you should be feeding it as a variable through the constructor (and saved as a variable within the class for access by any other methods) or directly to the method itself.

As fret was saying, this allows your library to be completely unaware of any global variables. It doesn’t care where it gets ‘postedDeals’ - just that it is in a certain format.

Hello,

Thank you for your posts. A few things:

Before I ask questions, just to clarify this, my goal is to build a page where accepts data (post, xml,…) and sends the appropriate campaign. I hope we are on the same page, I am pretty sure we are.

1 - So can I say, repository is the code ( class ) that provides data ( from database, xml, json, …) based on a specific request from our main controller.

In other words, lets say we use database ( and not xml,… ) so the repository accepts request id ( for example campaign id ) from a controller, retrieves the results from the database by calling model and passing on the right id, and sends the results back to the controller. Correct?

2 - What are other types of data here? Besides databases, xml and json.

3 - About entity classes. Does it mean the main class? For example if the page is mysite.com/send_campaign and the class stored in the controllers directory is called send_campaign …then is this the entity class?

About your code:

class CampaignController {

    public function send($campaignId)
    {
        $dealIds = Input::get('deals');
        $title   = Input::get('title');

        $service = new SendCampaign(new CampaignRepository, new DealRepository, new TemplateLoader, new Mailer);

        try {
            $service->send($campaignId, $dealIds, $title);
        } catch (Exception $e) {
            // Get error msg from exception and do something with it
        }

        // Display a 'success view' / redirect
    }

}  

4 - I am very sorry but I still don’t get input::get(‘deals’). Looks like you have a class called input with a static method get? If so what exactly do you put in there?

5 - By the way is input a service?

6 - Is mailer a service? If so why don’t you call it mailer service?

7 - Isn’t this part coupling? You have put this directly in your controller.

 $service = new SendCampaign(new CampaignRepository, new DealRepository, new TemplateLoader, new Mailer); 

Thank you :slight_smile:

I think you might find this article interesting. It does into more depth on the repository pattern, and will also provide some background to some of the other things we’ve talked about.

It’s not about the types of data so much as where the data comes from. It might be from different SQL databases (MySQL, Postgres, MSSQL), NoSQL databases (Mongo, CouchDB), or an external API. A repository could interface with any kind of persistence system.

No, an entity is an object that represents a concept in your application that has an identity. The most common type of entity in a web app is a User. We say that the User has identity, because even if we change properties of the User (such as their name, or their email address) it’s still the same User. If you were building an accounting app, you’d probably have an Invoice entity, for example.

Input is a helper class provided by Laravel. As your first example in the other thread (re: the single responsibility principle) used the class, I thought you were already familiar with it. You could just as easily do something like this:


$dealIds = isset($_POST['deals']) ? $_POST['deals'] : array();
$title   = isset($_POST['title']) ? $_POST['title'] : '';

It is, and yes you could call it MailerService if you preferred. I just kept the name over from your code, as I assumed the use of your mailer class in my example.

Yes it is. With Laravel you could use the built-in DI container to inject the pre-configured SendCampaign service into your controller if you wanted, which would decouple things further. I was just keeping it simple in my example.

There might be a little confusion here due to the fact that your seeing this happen within a class rather than following what we’ve been stressing, using DI (dependency injection). Keep in mind that this is happening in the controller, which is where all your interaction between your models and the user should occur. This is where your “procedural stuff” would go.

Hi,

I am reading on the resources you provided, I need to read and understand them, it might be a few days before I respond to this.

Thanks

Well it takes me months to read DDD book. I think you need to keep watching tutorials :slight_smile:

Thank you so much for your helps, learned a lot so far.

Let’s see if we can wrap it up and get some conclusion. The code in post #10 seems to be a good approach to pages with many functionalities; lots going on in there though.

1 - Main controller seems to be working like a “core” that is NOT responsible for doing database interactions or doing any service work like sending emails etc.

It is only responsible to work with repositories and services. Instantiates the right repositories and services and get them to do the work.

2 - The work should be spread between repositories and services rather than being in 1 place.

At the end how do you describe SendCampaign class in post 10? It is taking a lot of pressure of the main controller.

It is responsible for a lot of things. Is it a repository / service provider?

Thanks

Right, your controllers should not be doing any of the work. They take input from the request, pass it to the model (which does the work), and pass data from the model to the view.

Or to put it slightly differently, the work should be done in your model layer not in your controller.

Just be aware of the distinction between terms: a service provider is something different from a service. SendCampaign is a service not a repository, as it doesn’t deal with the storage or retrieval of entities (whereas CampaignRepository clearly does do this).

The class actually only has a single responsibility - to coordinate the sending of a campaign. All of the actual work is delegated to other objects. As long as you’re coding to interfaces, the implementation of any of the classes can be changed without affecting the SendCampaign class.

Hi, thanks for not giving up on me. I have learned a lot of new things so far! I would appreciate if you answer a few more questions please.

This might not be the best place to bring in MVC but I want to use this opportunity and clear things up even more. This is how I learned things:

To take it to next level, separate tasks between the repositories and services, I see it like this:

1 - Please comment on these 2 above. Are they correct? Or you would move things around?

2 - What do you mean by model layer?

You mentioned " the work should be done in your model layer not in your controller".

Model, I always thought about it as the connection between the controller and the database. “Models are PHP classes that are designed to work with information in your database. For example, let’s say you use CodeIgniter to manage a blog. You might have a model class that contains functions to insert, update, and retrieve your blog data.”

Do you agree with that? If so you probably agree with my diagrams above. If not please explain.

3 - Perhaps “model layer” is different from “model”. Isn’t it?

4 - Would it be possible to make a small diagram of what I am trying to show lol. Unless my diagram is spot on.

5 - You mentioned

Just be aware of the distinction between terms: a service provider is something different from a service. SendCampaign is a service not a repository, as it doesn’t deal with the storage or retrieval of entities (whereas CampaignRepository clearly does do this).

Would please confirm this:
Repositories deal with CRUD
Services do things like sending email
Service providers, I don’t know what they are.

Thank you very much

I can’t edit my post above for some reason! Post 19 is very important :frowning: I just add the left overs here:

6 - In a few sentences can you please explain what is persistence layer and how is it related to model layer? Are there any other layers? Domain layer maybe?

7 - Would you please explain this:

Favour composition over inheritance.

8 - Abstraction! What is it really? I understand it is the fundamental of OOP. I have a hard time understanding this. The irony is that I believe I am applying it!

Thank you