1
\$\begingroup\$

I am learning decorator design pattern in PHP. Please see what i am doing is correct or wrong, would appreciate your review, feedback, comments, suggestions.

<?php
/**
 * NotifierService interface is used as abstraction for Notification Mechanism
 *
 * It can be later implemented as EmailNotifierService, PushNotifierService, or
 * any other Notification SErvice
 */
interface NotifierService {
 public function Notify();
}
/**
 * EmailNotifierService - is Email Decorator
 * here the EmailSending is Implemented, and it can be used wherever it is
 * required, Different Unique services that doesn not have common behaviours
 * are utilised using decorator patterns
 */
class EmailNotifierService implements NotifierService {
 public function __construct(){
 $this->to = '[email protected]' ;//$toarr;
 $this->from = '[email protected]' ;//$from;
 $this->msg = 'This is a test message';//$msg
 }
 /**
 * [sendMail Actual Implementation of Sending Email is put here]
 * Kept simple for demo purposes
 */
 private function sendMail(){
 echo "Sending Email To" .PHP_EOL;
 print_r($this->to);
 echo "From: " .PHP_EOL;
 print_r($this->from);
 echo "Msg: " .PHP_EOL;
 print_r($this->msg);
 }
 /**
 * [Notify - Implementation of the interface method - Notify]
 */
 public function Notify(){
 echo "Notify function called" .PHP_EOL;
 $this->SendMail();
 }
}
class User implements NotifierService{
 var $users = [];
 var $notifierservice;
 //Passing NotifierService Object, So that we can utilise its service
 public function __construct(NotifierService $notifierservice){
 echo "New User constructed";
 $this->notifierservice = $notifierservice;
 }
 /**
 * [adduser This function creates a new user, like new user signup,
 * Whenever a new user signup, a Email is sent to the Website Admin]
 * @param [Username] $u Username
 */
 public function adduser($u){
 $this->users[]=$u;
 $this->Notify();// Calls the Notification
 }
 public function Notify(){
 $this->notifierservice->Notify();
 }
}
// New User is created, EmailNotificationService is wrapped in it
// So that whenever a new user is created, Admin gets an Email
$u1 = new User(new EmailNotifierService());
$u1->adduser('Ramkumar');
200_success
145k22 gold badges190 silver badges478 bronze badges
asked Apr 3, 2017 at 11:51
\$\endgroup\$
1

1 Answer 1

2
\$\begingroup\$
  • Just because a class like User has a dependency on another class (like an objects instance of type NotifierService) it does not mean that class has to implement that same interface. Here it seems that User should not implement this interface at all. You can remove the Notify() method on this class and just call the Notify() method on the passed dependency in your adduser() method.
  • I don't think decorator pattern makes sense at all for what you are trying to do here. You are not really trying to "decorate" objects adhering to NotifierService interface with additional functionality related to that interface, but rather simply consume a class of this interface in User. This is a straight dependency injection situation.
  • The class implementation of EmailNotifierService is pretty trivial here with hard-coded to/from/message values, almost to the point that this code does not meet code review criteria. Surely this is not something that is production ready.
  • Don't echo out output from classes such as this. Leave that to some other functionality. If you simply want to "log" out class activity, then actually log it (you know to application/server logs).
  • Why does a class called User deal with multiple users? Should this class be renamed?
  • Don't start method/function names with uppercase letters like you do here with Notify() and SendMail(). This should be notify() and sendMail().
  • $notifierservice should probably be $notifierService (camel-cased). adduser() should be addUser().
  • You should consider adding validation on public methods, either through type hinting of parameters (like you do for the notifier dependency) or through guarding clauses at beginning of method. Here, for example, you do nothing to ensure you are getting a valid user passed to your adduser() method.
answered Apr 3, 2017 at 13:58
\$\endgroup\$
1
  • \$\begingroup\$ Thanks a lot for every of your comments. +1 for Add validation on public methods with typehinting. \$\endgroup\$ Commented Apr 6, 2017 at 3:40

Your Answer

Draft saved
Draft discarded

Sign up or log in

Sign up using Google
Sign up using Email and Password

Post as a guest

Required, but never shown

Post as a guest

Required, but never shown

By clicking "Post Your Answer", you agree to our terms of service and acknowledge you have read our privacy policy.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.