Refactoring my controllers with a BaseController to remove repetitive code?












0












$begingroup$


Take this controller for instance. Is there a way I can rip out the constructor and property, extend a base controller class and still have $twig? I feel like this repeitive behaviour can be removed.



I understand that I need to extend a base class, but I'm not 100% sure where to go from there.



namespace AppControllers;

class Welcome extends {
private $twig;

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

public function getTemplate() {
return $this->twig->render('/templates/welcome.html', array());
}
}

?>


When I try and create a base class like this, removing the constructor and property from the Welcome class, I get Undefined property: AppControllersWelcome::$twig



<?php declare(strict_types = 1);

namespace AppControllers;

class Base {
private $twig;

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

}

?>


If I have 100 controllers, that's 100 classes to refactor if I ever change my template engine, surely there must be a way to add some sort of base class to do this? And then extend it in all classes.










share|improve this question









$endgroup$

















    0












    $begingroup$


    Take this controller for instance. Is there a way I can rip out the constructor and property, extend a base controller class and still have $twig? I feel like this repeitive behaviour can be removed.



    I understand that I need to extend a base class, but I'm not 100% sure where to go from there.



    namespace AppControllers;

    class Welcome extends {
    private $twig;

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

    public function getTemplate() {
    return $this->twig->render('/templates/welcome.html', array());
    }
    }

    ?>


    When I try and create a base class like this, removing the constructor and property from the Welcome class, I get Undefined property: AppControllersWelcome::$twig



    <?php declare(strict_types = 1);

    namespace AppControllers;

    class Base {
    private $twig;

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

    }

    ?>


    If I have 100 controllers, that's 100 classes to refactor if I ever change my template engine, surely there must be a way to add some sort of base class to do this? And then extend it in all classes.










    share|improve this question









    $endgroup$















      0












      0








      0





      $begingroup$


      Take this controller for instance. Is there a way I can rip out the constructor and property, extend a base controller class and still have $twig? I feel like this repeitive behaviour can be removed.



      I understand that I need to extend a base class, but I'm not 100% sure where to go from there.



      namespace AppControllers;

      class Welcome extends {
      private $twig;

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

      public function getTemplate() {
      return $this->twig->render('/templates/welcome.html', array());
      }
      }

      ?>


      When I try and create a base class like this, removing the constructor and property from the Welcome class, I get Undefined property: AppControllersWelcome::$twig



      <?php declare(strict_types = 1);

      namespace AppControllers;

      class Base {
      private $twig;

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

      }

      ?>


      If I have 100 controllers, that's 100 classes to refactor if I ever change my template engine, surely there must be a way to add some sort of base class to do this? And then extend it in all classes.










      share|improve this question









      $endgroup$




      Take this controller for instance. Is there a way I can rip out the constructor and property, extend a base controller class and still have $twig? I feel like this repeitive behaviour can be removed.



      I understand that I need to extend a base class, but I'm not 100% sure where to go from there.



      namespace AppControllers;

      class Welcome extends {
      private $twig;

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

      public function getTemplate() {
      return $this->twig->render('/templates/welcome.html', array());
      }
      }

      ?>


      When I try and create a base class like this, removing the constructor and property from the Welcome class, I get Undefined property: AppControllersWelcome::$twig



      <?php declare(strict_types = 1);

      namespace AppControllers;

      class Base {
      private $twig;

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

      }

      ?>


      If I have 100 controllers, that's 100 classes to refactor if I ever change my template engine, surely there must be a way to add some sort of base class to do this? And then extend it in all classes.







      php






      share|improve this question













      share|improve this question











      share|improve this question




      share|improve this question










      asked 9 hours ago









      Adam GAdam G

      291




      291






















          2 Answers
          2






          active

          oldest

          votes


















          1












          $begingroup$

          You can make a 'getter' method for $twig in your base controller, like this:



          class Base {
          private $twig;

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

          public function getTwig()
          {
          return $this->twig;
          }

          }


          This would make the getTemplate() method look like this:



          public function getTemplate() {
          return $this->getTwig()->render('/templates/welcome.html', array());
          }


          Or you could make $twig protected instead of private. See:



          https://www.php.net/manual/en/language.oop5.visibility.php



          Your code is far from complete, and so my answer is too. There may be other ways to do things. Always keep in might that ownership of classes should guide your decisions. For instance, if you don't own the twig class you could use the base controller to interface with it. In that case you would not make a getter for $twig, but implement a method for render(), inside the base controllor, which can be used by extended classes. That way you will only ever need to change the base controller if the twig class changes.






          share|improve this answer











          $endgroup$





















            1












            $begingroup$

            Lets look at your code



            namespace AppControllers;

            class Welcome extends {
            private $twig;

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

            public function getTemplate() {
            return $this->twig->render('/templates/welcome.html', array());
            }
            }
            //------------------------
            namespace AppControllers;

            class Base {
            private $twig;

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

            }


            You have 3 Big issues here:





            1. class Welcome extends { - extends what


            2. private $twig; this property is private, which means only the class it's defined in can access it

            3. public function __construct


            Lets fix these (this simplest way):



            namespace AppControllers;
            class Welcome extends Base {
            protected $twig; //use protected - even though private would work this way

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

            public function getTemplate() {
            return $this->twig->render('/templates/welcome.html', array());
            }
            }
            //------------------------
            namespace AppControllers;

            class Base {

            /*public function __construct(Twig_Environment $twig) {
            parent::__construct($twig); //or omit the constructor all togather
            }*/

            }


            In this simple example, you can just Nuke the guts of the child class as there is nothing specific to that class in the code shown. This happens to be the whole point of inheritance.



            I left the constructor in comments, in this minimal example its not needed. But you may want it in the future, so you can use parent to call the parent classes version of any protected/public methods. Regardless if $twig is private we will want to try to re-use as much of the parent code as we can. If it's private you have no choice but to use the parent's version, or set it after the fact.



            If you want to keep $twig private for some reason, keep in mind the property is only visible within the Base class. So any work you do directly on it must also be in that same class. This means you can setup a method to return it, or set it in the parent class and call it from the child class. This is rather trivial so I don't think it warrants an example, but if you want one, just let me know.



            To be honest I use private about 10% of the time. Only when I have some value only the parent class should "know" about. And example would be say you have a base class that connects to the DB. Well the act of connecting and the data needed for that is largely irrelevant to the child classes. They don't (nor should they) care how the DB connection happened as the parent can take care of that without any need for the child to be aware of it. All the child needs is whatever connection resource you get from the act of connecting.



            When thinking about OOP, one very good thing to learn is called S.O.L.I.D



            https://en.wikipedia.org/wiki/SOLID




            Single responsibility principle[6]
            A class should have only a single responsibility, that is, only changes to one part of the software's specification should be able to affect the specification of the class.



            Open architecture "Software entities ... should be open for extension, but closed for modification."



            Liskov substitution principle
            "Objects in a program should be replaceable with instances of their subtypes without altering the correctness of that program." See also design by contract.



            Interface segregation principle "Many client-specific interfaces are better than one general-purpose interface."



            Dependency inversion principle
            One should "depend upon abstractions, [not] concretions."




            By having the template object private, your breaking rule Number2 or the Open architecture. You could make Base an Abstract class (if it doesn't extend a concrete class), this way your not tempted to use it as a concrete Object. That's mostly 5. If the parent is "responsible" for creating the template, there is no need for the child to take on that responsibility 1 and some of 3 (by not duplicating the code, we insure that Welcome can seamlessly replace Base etc... Because this is not 100% code you control, a Controller must meet certain specifications for example, there is only so much you can do. A controller is also generally the end result of a Request so you won't have much need of interfaces etc. Because you can't just load a controller at will and use it. Interfaces are for type hinting your inputs to insure they posses the methods the interface demands. That is the contract for the object.



            These are "general" guide lines, you should try to use. But that doesn't mean you have to use all of them. For example don't make abstract classes and interfaces just because they are part of SOLID principals, due it because it's the correct thing to do for that object.



            Cheers!






            share|improve this answer











            $endgroup$













              Your Answer





              StackExchange.ifUsing("editor", function () {
              return StackExchange.using("mathjaxEditing", function () {
              StackExchange.MarkdownEditor.creationCallbacks.add(function (editor, postfix) {
              StackExchange.mathjaxEditing.prepareWmdForMathJax(editor, postfix, [["\$", "\$"]]);
              });
              });
              }, "mathjax-editing");

              StackExchange.ifUsing("editor", function () {
              StackExchange.using("externalEditor", function () {
              StackExchange.using("snippets", function () {
              StackExchange.snippets.init();
              });
              });
              }, "code-snippets");

              StackExchange.ready(function() {
              var channelOptions = {
              tags: "".split(" "),
              id: "196"
              };
              initTagRenderer("".split(" "), "".split(" "), channelOptions);

              StackExchange.using("externalEditor", function() {
              // Have to fire editor after snippets, if snippets enabled
              if (StackExchange.settings.snippets.snippetsEnabled) {
              StackExchange.using("snippets", function() {
              createEditor();
              });
              }
              else {
              createEditor();
              }
              });

              function createEditor() {
              StackExchange.prepareEditor({
              heartbeatType: 'answer',
              autoActivateHeartbeat: false,
              convertImagesToLinks: false,
              noModals: true,
              showLowRepImageUploadWarning: true,
              reputationToPostImages: null,
              bindNavPrevention: true,
              postfix: "",
              imageUploader: {
              brandingHtml: "Powered by u003ca class="icon-imgur-white" href="https://imgur.com/"u003eu003c/au003e",
              contentPolicyHtml: "User contributions licensed under u003ca href="https://creativecommons.org/licenses/by-sa/3.0/"u003ecc by-sa 3.0 with attribution requiredu003c/au003e u003ca href="https://stackoverflow.com/legal/content-policy"u003e(content policy)u003c/au003e",
              allowUrls: true
              },
              onDemand: true,
              discardSelector: ".discard-answer"
              ,immediatelyShowMarkdownHelp:true
              });


              }
              });














              draft saved

              draft discarded


















              StackExchange.ready(
              function () {
              StackExchange.openid.initPostLogin('.new-post-login', 'https%3a%2f%2fcodereview.stackexchange.com%2fquestions%2f216269%2frefactoring-my-controllers-with-a-basecontroller-to-remove-repetitive-code%23new-answer', 'question_page');
              }
              );

              Post as a guest















              Required, but never shown

























              2 Answers
              2






              active

              oldest

              votes








              2 Answers
              2






              active

              oldest

              votes









              active

              oldest

              votes






              active

              oldest

              votes









              1












              $begingroup$

              You can make a 'getter' method for $twig in your base controller, like this:



              class Base {
              private $twig;

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

              public function getTwig()
              {
              return $this->twig;
              }

              }


              This would make the getTemplate() method look like this:



              public function getTemplate() {
              return $this->getTwig()->render('/templates/welcome.html', array());
              }


              Or you could make $twig protected instead of private. See:



              https://www.php.net/manual/en/language.oop5.visibility.php



              Your code is far from complete, and so my answer is too. There may be other ways to do things. Always keep in might that ownership of classes should guide your decisions. For instance, if you don't own the twig class you could use the base controller to interface with it. In that case you would not make a getter for $twig, but implement a method for render(), inside the base controllor, which can be used by extended classes. That way you will only ever need to change the base controller if the twig class changes.






              share|improve this answer











              $endgroup$


















                1












                $begingroup$

                You can make a 'getter' method for $twig in your base controller, like this:



                class Base {
                private $twig;

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

                public function getTwig()
                {
                return $this->twig;
                }

                }


                This would make the getTemplate() method look like this:



                public function getTemplate() {
                return $this->getTwig()->render('/templates/welcome.html', array());
                }


                Or you could make $twig protected instead of private. See:



                https://www.php.net/manual/en/language.oop5.visibility.php



                Your code is far from complete, and so my answer is too. There may be other ways to do things. Always keep in might that ownership of classes should guide your decisions. For instance, if you don't own the twig class you could use the base controller to interface with it. In that case you would not make a getter for $twig, but implement a method for render(), inside the base controllor, which can be used by extended classes. That way you will only ever need to change the base controller if the twig class changes.






                share|improve this answer











                $endgroup$
















                  1












                  1








                  1





                  $begingroup$

                  You can make a 'getter' method for $twig in your base controller, like this:



                  class Base {
                  private $twig;

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

                  public function getTwig()
                  {
                  return $this->twig;
                  }

                  }


                  This would make the getTemplate() method look like this:



                  public function getTemplate() {
                  return $this->getTwig()->render('/templates/welcome.html', array());
                  }


                  Or you could make $twig protected instead of private. See:



                  https://www.php.net/manual/en/language.oop5.visibility.php



                  Your code is far from complete, and so my answer is too. There may be other ways to do things. Always keep in might that ownership of classes should guide your decisions. For instance, if you don't own the twig class you could use the base controller to interface with it. In that case you would not make a getter for $twig, but implement a method for render(), inside the base controllor, which can be used by extended classes. That way you will only ever need to change the base controller if the twig class changes.






                  share|improve this answer











                  $endgroup$



                  You can make a 'getter' method for $twig in your base controller, like this:



                  class Base {
                  private $twig;

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

                  public function getTwig()
                  {
                  return $this->twig;
                  }

                  }


                  This would make the getTemplate() method look like this:



                  public function getTemplate() {
                  return $this->getTwig()->render('/templates/welcome.html', array());
                  }


                  Or you could make $twig protected instead of private. See:



                  https://www.php.net/manual/en/language.oop5.visibility.php



                  Your code is far from complete, and so my answer is too. There may be other ways to do things. Always keep in might that ownership of classes should guide your decisions. For instance, if you don't own the twig class you could use the base controller to interface with it. In that case you would not make a getter for $twig, but implement a method for render(), inside the base controllor, which can be used by extended classes. That way you will only ever need to change the base controller if the twig class changes.







                  share|improve this answer














                  share|improve this answer



                  share|improve this answer








                  edited 6 hours ago

























                  answered 6 hours ago









                  KIKO SoftwareKIKO Software

                  1,579512




                  1,579512

























                      1












                      $begingroup$

                      Lets look at your code



                      namespace AppControllers;

                      class Welcome extends {
                      private $twig;

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

                      public function getTemplate() {
                      return $this->twig->render('/templates/welcome.html', array());
                      }
                      }
                      //------------------------
                      namespace AppControllers;

                      class Base {
                      private $twig;

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

                      }


                      You have 3 Big issues here:





                      1. class Welcome extends { - extends what


                      2. private $twig; this property is private, which means only the class it's defined in can access it

                      3. public function __construct


                      Lets fix these (this simplest way):



                      namespace AppControllers;
                      class Welcome extends Base {
                      protected $twig; //use protected - even though private would work this way

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

                      public function getTemplate() {
                      return $this->twig->render('/templates/welcome.html', array());
                      }
                      }
                      //------------------------
                      namespace AppControllers;

                      class Base {

                      /*public function __construct(Twig_Environment $twig) {
                      parent::__construct($twig); //or omit the constructor all togather
                      }*/

                      }


                      In this simple example, you can just Nuke the guts of the child class as there is nothing specific to that class in the code shown. This happens to be the whole point of inheritance.



                      I left the constructor in comments, in this minimal example its not needed. But you may want it in the future, so you can use parent to call the parent classes version of any protected/public methods. Regardless if $twig is private we will want to try to re-use as much of the parent code as we can. If it's private you have no choice but to use the parent's version, or set it after the fact.



                      If you want to keep $twig private for some reason, keep in mind the property is only visible within the Base class. So any work you do directly on it must also be in that same class. This means you can setup a method to return it, or set it in the parent class and call it from the child class. This is rather trivial so I don't think it warrants an example, but if you want one, just let me know.



                      To be honest I use private about 10% of the time. Only when I have some value only the parent class should "know" about. And example would be say you have a base class that connects to the DB. Well the act of connecting and the data needed for that is largely irrelevant to the child classes. They don't (nor should they) care how the DB connection happened as the parent can take care of that without any need for the child to be aware of it. All the child needs is whatever connection resource you get from the act of connecting.



                      When thinking about OOP, one very good thing to learn is called S.O.L.I.D



                      https://en.wikipedia.org/wiki/SOLID




                      Single responsibility principle[6]
                      A class should have only a single responsibility, that is, only changes to one part of the software's specification should be able to affect the specification of the class.



                      Open architecture "Software entities ... should be open for extension, but closed for modification."



                      Liskov substitution principle
                      "Objects in a program should be replaceable with instances of their subtypes without altering the correctness of that program." See also design by contract.



                      Interface segregation principle "Many client-specific interfaces are better than one general-purpose interface."



                      Dependency inversion principle
                      One should "depend upon abstractions, [not] concretions."




                      By having the template object private, your breaking rule Number2 or the Open architecture. You could make Base an Abstract class (if it doesn't extend a concrete class), this way your not tempted to use it as a concrete Object. That's mostly 5. If the parent is "responsible" for creating the template, there is no need for the child to take on that responsibility 1 and some of 3 (by not duplicating the code, we insure that Welcome can seamlessly replace Base etc... Because this is not 100% code you control, a Controller must meet certain specifications for example, there is only so much you can do. A controller is also generally the end result of a Request so you won't have much need of interfaces etc. Because you can't just load a controller at will and use it. Interfaces are for type hinting your inputs to insure they posses the methods the interface demands. That is the contract for the object.



                      These are "general" guide lines, you should try to use. But that doesn't mean you have to use all of them. For example don't make abstract classes and interfaces just because they are part of SOLID principals, due it because it's the correct thing to do for that object.



                      Cheers!






                      share|improve this answer











                      $endgroup$


















                        1












                        $begingroup$

                        Lets look at your code



                        namespace AppControllers;

                        class Welcome extends {
                        private $twig;

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

                        public function getTemplate() {
                        return $this->twig->render('/templates/welcome.html', array());
                        }
                        }
                        //------------------------
                        namespace AppControllers;

                        class Base {
                        private $twig;

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

                        }


                        You have 3 Big issues here:





                        1. class Welcome extends { - extends what


                        2. private $twig; this property is private, which means only the class it's defined in can access it

                        3. public function __construct


                        Lets fix these (this simplest way):



                        namespace AppControllers;
                        class Welcome extends Base {
                        protected $twig; //use protected - even though private would work this way

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

                        public function getTemplate() {
                        return $this->twig->render('/templates/welcome.html', array());
                        }
                        }
                        //------------------------
                        namespace AppControllers;

                        class Base {

                        /*public function __construct(Twig_Environment $twig) {
                        parent::__construct($twig); //or omit the constructor all togather
                        }*/

                        }


                        In this simple example, you can just Nuke the guts of the child class as there is nothing specific to that class in the code shown. This happens to be the whole point of inheritance.



                        I left the constructor in comments, in this minimal example its not needed. But you may want it in the future, so you can use parent to call the parent classes version of any protected/public methods. Regardless if $twig is private we will want to try to re-use as much of the parent code as we can. If it's private you have no choice but to use the parent's version, or set it after the fact.



                        If you want to keep $twig private for some reason, keep in mind the property is only visible within the Base class. So any work you do directly on it must also be in that same class. This means you can setup a method to return it, or set it in the parent class and call it from the child class. This is rather trivial so I don't think it warrants an example, but if you want one, just let me know.



                        To be honest I use private about 10% of the time. Only when I have some value only the parent class should "know" about. And example would be say you have a base class that connects to the DB. Well the act of connecting and the data needed for that is largely irrelevant to the child classes. They don't (nor should they) care how the DB connection happened as the parent can take care of that without any need for the child to be aware of it. All the child needs is whatever connection resource you get from the act of connecting.



                        When thinking about OOP, one very good thing to learn is called S.O.L.I.D



                        https://en.wikipedia.org/wiki/SOLID




                        Single responsibility principle[6]
                        A class should have only a single responsibility, that is, only changes to one part of the software's specification should be able to affect the specification of the class.



                        Open architecture "Software entities ... should be open for extension, but closed for modification."



                        Liskov substitution principle
                        "Objects in a program should be replaceable with instances of their subtypes without altering the correctness of that program." See also design by contract.



                        Interface segregation principle "Many client-specific interfaces are better than one general-purpose interface."



                        Dependency inversion principle
                        One should "depend upon abstractions, [not] concretions."




                        By having the template object private, your breaking rule Number2 or the Open architecture. You could make Base an Abstract class (if it doesn't extend a concrete class), this way your not tempted to use it as a concrete Object. That's mostly 5. If the parent is "responsible" for creating the template, there is no need for the child to take on that responsibility 1 and some of 3 (by not duplicating the code, we insure that Welcome can seamlessly replace Base etc... Because this is not 100% code you control, a Controller must meet certain specifications for example, there is only so much you can do. A controller is also generally the end result of a Request so you won't have much need of interfaces etc. Because you can't just load a controller at will and use it. Interfaces are for type hinting your inputs to insure they posses the methods the interface demands. That is the contract for the object.



                        These are "general" guide lines, you should try to use. But that doesn't mean you have to use all of them. For example don't make abstract classes and interfaces just because they are part of SOLID principals, due it because it's the correct thing to do for that object.



                        Cheers!






                        share|improve this answer











                        $endgroup$
















                          1












                          1








                          1





                          $begingroup$

                          Lets look at your code



                          namespace AppControllers;

                          class Welcome extends {
                          private $twig;

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

                          public function getTemplate() {
                          return $this->twig->render('/templates/welcome.html', array());
                          }
                          }
                          //------------------------
                          namespace AppControllers;

                          class Base {
                          private $twig;

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

                          }


                          You have 3 Big issues here:





                          1. class Welcome extends { - extends what


                          2. private $twig; this property is private, which means only the class it's defined in can access it

                          3. public function __construct


                          Lets fix these (this simplest way):



                          namespace AppControllers;
                          class Welcome extends Base {
                          protected $twig; //use protected - even though private would work this way

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

                          public function getTemplate() {
                          return $this->twig->render('/templates/welcome.html', array());
                          }
                          }
                          //------------------------
                          namespace AppControllers;

                          class Base {

                          /*public function __construct(Twig_Environment $twig) {
                          parent::__construct($twig); //or omit the constructor all togather
                          }*/

                          }


                          In this simple example, you can just Nuke the guts of the child class as there is nothing specific to that class in the code shown. This happens to be the whole point of inheritance.



                          I left the constructor in comments, in this minimal example its not needed. But you may want it in the future, so you can use parent to call the parent classes version of any protected/public methods. Regardless if $twig is private we will want to try to re-use as much of the parent code as we can. If it's private you have no choice but to use the parent's version, or set it after the fact.



                          If you want to keep $twig private for some reason, keep in mind the property is only visible within the Base class. So any work you do directly on it must also be in that same class. This means you can setup a method to return it, or set it in the parent class and call it from the child class. This is rather trivial so I don't think it warrants an example, but if you want one, just let me know.



                          To be honest I use private about 10% of the time. Only when I have some value only the parent class should "know" about. And example would be say you have a base class that connects to the DB. Well the act of connecting and the data needed for that is largely irrelevant to the child classes. They don't (nor should they) care how the DB connection happened as the parent can take care of that without any need for the child to be aware of it. All the child needs is whatever connection resource you get from the act of connecting.



                          When thinking about OOP, one very good thing to learn is called S.O.L.I.D



                          https://en.wikipedia.org/wiki/SOLID




                          Single responsibility principle[6]
                          A class should have only a single responsibility, that is, only changes to one part of the software's specification should be able to affect the specification of the class.



                          Open architecture "Software entities ... should be open for extension, but closed for modification."



                          Liskov substitution principle
                          "Objects in a program should be replaceable with instances of their subtypes without altering the correctness of that program." See also design by contract.



                          Interface segregation principle "Many client-specific interfaces are better than one general-purpose interface."



                          Dependency inversion principle
                          One should "depend upon abstractions, [not] concretions."




                          By having the template object private, your breaking rule Number2 or the Open architecture. You could make Base an Abstract class (if it doesn't extend a concrete class), this way your not tempted to use it as a concrete Object. That's mostly 5. If the parent is "responsible" for creating the template, there is no need for the child to take on that responsibility 1 and some of 3 (by not duplicating the code, we insure that Welcome can seamlessly replace Base etc... Because this is not 100% code you control, a Controller must meet certain specifications for example, there is only so much you can do. A controller is also generally the end result of a Request so you won't have much need of interfaces etc. Because you can't just load a controller at will and use it. Interfaces are for type hinting your inputs to insure they posses the methods the interface demands. That is the contract for the object.



                          These are "general" guide lines, you should try to use. But that doesn't mean you have to use all of them. For example don't make abstract classes and interfaces just because they are part of SOLID principals, due it because it's the correct thing to do for that object.



                          Cheers!






                          share|improve this answer











                          $endgroup$



                          Lets look at your code



                          namespace AppControllers;

                          class Welcome extends {
                          private $twig;

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

                          public function getTemplate() {
                          return $this->twig->render('/templates/welcome.html', array());
                          }
                          }
                          //------------------------
                          namespace AppControllers;

                          class Base {
                          private $twig;

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

                          }


                          You have 3 Big issues here:





                          1. class Welcome extends { - extends what


                          2. private $twig; this property is private, which means only the class it's defined in can access it

                          3. public function __construct


                          Lets fix these (this simplest way):



                          namespace AppControllers;
                          class Welcome extends Base {
                          protected $twig; //use protected - even though private would work this way

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

                          public function getTemplate() {
                          return $this->twig->render('/templates/welcome.html', array());
                          }
                          }
                          //------------------------
                          namespace AppControllers;

                          class Base {

                          /*public function __construct(Twig_Environment $twig) {
                          parent::__construct($twig); //or omit the constructor all togather
                          }*/

                          }


                          In this simple example, you can just Nuke the guts of the child class as there is nothing specific to that class in the code shown. This happens to be the whole point of inheritance.



                          I left the constructor in comments, in this minimal example its not needed. But you may want it in the future, so you can use parent to call the parent classes version of any protected/public methods. Regardless if $twig is private we will want to try to re-use as much of the parent code as we can. If it's private you have no choice but to use the parent's version, or set it after the fact.



                          If you want to keep $twig private for some reason, keep in mind the property is only visible within the Base class. So any work you do directly on it must also be in that same class. This means you can setup a method to return it, or set it in the parent class and call it from the child class. This is rather trivial so I don't think it warrants an example, but if you want one, just let me know.



                          To be honest I use private about 10% of the time. Only when I have some value only the parent class should "know" about. And example would be say you have a base class that connects to the DB. Well the act of connecting and the data needed for that is largely irrelevant to the child classes. They don't (nor should they) care how the DB connection happened as the parent can take care of that without any need for the child to be aware of it. All the child needs is whatever connection resource you get from the act of connecting.



                          When thinking about OOP, one very good thing to learn is called S.O.L.I.D



                          https://en.wikipedia.org/wiki/SOLID




                          Single responsibility principle[6]
                          A class should have only a single responsibility, that is, only changes to one part of the software's specification should be able to affect the specification of the class.



                          Open architecture "Software entities ... should be open for extension, but closed for modification."



                          Liskov substitution principle
                          "Objects in a program should be replaceable with instances of their subtypes without altering the correctness of that program." See also design by contract.



                          Interface segregation principle "Many client-specific interfaces are better than one general-purpose interface."



                          Dependency inversion principle
                          One should "depend upon abstractions, [not] concretions."




                          By having the template object private, your breaking rule Number2 or the Open architecture. You could make Base an Abstract class (if it doesn't extend a concrete class), this way your not tempted to use it as a concrete Object. That's mostly 5. If the parent is "responsible" for creating the template, there is no need for the child to take on that responsibility 1 and some of 3 (by not duplicating the code, we insure that Welcome can seamlessly replace Base etc... Because this is not 100% code you control, a Controller must meet certain specifications for example, there is only so much you can do. A controller is also generally the end result of a Request so you won't have much need of interfaces etc. Because you can't just load a controller at will and use it. Interfaces are for type hinting your inputs to insure they posses the methods the interface demands. That is the contract for the object.



                          These are "general" guide lines, you should try to use. But that doesn't mean you have to use all of them. For example don't make abstract classes and interfaces just because they are part of SOLID principals, due it because it's the correct thing to do for that object.



                          Cheers!







                          share|improve this answer














                          share|improve this answer



                          share|improve this answer








                          edited 3 hours ago

























                          answered 3 hours ago









                          ArtisticPhoenixArtisticPhoenix

                          43127




                          43127






























                              draft saved

                              draft discarded




















































                              Thanks for contributing an answer to Code Review Stack Exchange!


                              • Please be sure to answer the question. Provide details and share your research!

                              But avoid



                              • Asking for help, clarification, or responding to other answers.

                              • Making statements based on opinion; back them up with references or personal experience.


                              Use MathJax to format equations. MathJax reference.


                              To learn more, see our tips on writing great answers.




                              draft saved


                              draft discarded














                              StackExchange.ready(
                              function () {
                              StackExchange.openid.initPostLogin('.new-post-login', 'https%3a%2f%2fcodereview.stackexchange.com%2fquestions%2f216269%2frefactoring-my-controllers-with-a-basecontroller-to-remove-repetitive-code%23new-answer', 'question_page');
                              }
                              );

                              Post as a guest















                              Required, but never shown





















































                              Required, but never shown














                              Required, but never shown












                              Required, but never shown







                              Required, but never shown

































                              Required, but never shown














                              Required, but never shown












                              Required, but never shown







                              Required, but never shown







                              Popular posts from this blog

                              How to reconfigure Docker Trusted Registry 2.x.x to use CEPH FS mount instead of NFS and other traditional...

                              is 'sed' thread safe

                              How to make a Squid Proxy server?