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 = 'Praveen@gmail.com' ;//$toarr;
        $this->from  = 'Napoleon@gmail.com' ;//$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');
\$\endgroup\$
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.
\$\endgroup\$
1
  • \$\begingroup\$ Thanks a lot for every of your comments. +1 for Add validation on public methods with typehinting. \$\endgroup\$ – indianwebdevil Apr 6 '17 at 3:40

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.