Refactoring my controllers with a BaseController to remove repetitive code?
$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.
php
$endgroup$
add a comment |
$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.
php
$endgroup$
add a comment |
$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.
php
$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
php
asked 9 hours ago
Adam GAdam G
291
291
add a comment |
add a comment |
2 Answers
2
active
oldest
votes
$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.
$endgroup$
add a comment |
$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:
class Welcome extends {
- extends what
private $twig;
this property is private, which means only the class it's defined in can access itpublic 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!
$endgroup$
add a comment |
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
});
}
});
Sign up or log in
StackExchange.ready(function () {
StackExchange.helpers.onClickDraftSave('#login-link');
});
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Post as a guest
Required, but never shown
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
$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.
$endgroup$
add a comment |
$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.
$endgroup$
add a comment |
$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.
$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.
edited 6 hours ago
answered 6 hours ago
KIKO SoftwareKIKO Software
1,579512
1,579512
add a comment |
add a comment |
$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:
class Welcome extends {
- extends what
private $twig;
this property is private, which means only the class it's defined in can access itpublic 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!
$endgroup$
add a comment |
$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:
class Welcome extends {
- extends what
private $twig;
this property is private, which means only the class it's defined in can access itpublic 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!
$endgroup$
add a comment |
$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:
class Welcome extends {
- extends what
private $twig;
this property is private, which means only the class it's defined in can access itpublic 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!
$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:
class Welcome extends {
- extends what
private $twig;
this property is private, which means only the class it's defined in can access itpublic 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!
edited 3 hours ago
answered 3 hours ago
ArtisticPhoenixArtisticPhoenix
43127
43127
add a comment |
add a comment |
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.
Sign up or log in
StackExchange.ready(function () {
StackExchange.helpers.onClickDraftSave('#login-link');
});
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Post as a guest
Required, but never shown
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
Sign up or log in
StackExchange.ready(function () {
StackExchange.helpers.onClickDraftSave('#login-link');
});
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Post as a guest
Required, but never shown
Sign up or log in
StackExchange.ready(function () {
StackExchange.helpers.onClickDraftSave('#login-link');
});
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Post as a guest
Required, but never shown
Sign up or log in
StackExchange.ready(function () {
StackExchange.helpers.onClickDraftSave('#login-link');
});
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
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