removing Singletons, and just creating new objects when needed#11
removing Singletons, and just creating new objects when needed#11
Conversation
tomjn
left a comment
There was a problem hiding this comment.
I'd also recommend moving stuff out of the constructor to a dedicated run function. This way creating the object and using the object are separate tasks.
This also means in the root file we get a nice 3 step process:
- load the class files
- create and assemble the program from objects like lego
- tell it we're ready to run and go do things
| use GutenbergArray\API; | ||
| use GutenbergArray\Hooks; | ||
|
|
||
| class GutenbergObjectPlugin { |
There was a problem hiding this comment.
I would keep the gutenberg plugin object, it provides structure and gives all the other objects somewhere to live. Pass the other objects in via the constructor and store them as member variables
There was a problem hiding this comment.
Keep in mind though that this object should take all the other objects in as dependencies, and it shouldn't do anything itself that isn't a method call of its children.
src/Hooks.php
Outdated
| public function get_block_data( $post ) { | ||
| if ( ! $this->API ) { | ||
| $this->API = API::init(); | ||
| $this->API = new API(); |
There was a problem hiding this comment.
This leads to multiple API objects, instead eliminate this entire check, and change the constructor
src/Hooks.php
Outdated
|
|
||
| private $API; | ||
|
|
||
| public function __construct() { |
There was a problem hiding this comment.
Lets use dependency injection by changing the constructor, this object requires an API object to do it's work, so lets make it ask for one when it gets created:
public function __construct( API $api ) {
$this->api = $api;
}Now we don't need to check if api is set, if it isn't set the constructor will throw an error thanks to the type hinting
src/Hooks.php
Outdated
|
|
||
| use Singleton; | ||
|
|
||
| private $API; |
There was a problem hiding this comment.
Lets keep variables lowercase, and use uppercase for the Class names instead
|
The final bootstrap might look something like this: // definitions
namespace GutenbergArray;
define( 'GutesArraySaveUrl', plugin_dir_url( __FILE__ ) );
define( 'GutesArrayPluginFile', __FILE__ );
// load classes
if ( file_exists( __DIR__ . '/vendor/autoload.php' ) ) {
require_once 'vendor/autoload.php';
}
// construct plugin
$api = new API();
$hooks = new Hooks( $api );
$scripts = new Scripts();
$database = new Database();
$plugin = new GutenbergObjectPlugin( $hooks, $scripts, $database );
// run plugin, register hooks, etc
$plugin->run();Additionally, If the plugin got bigger, you could swap classes out for interfaces and build different implementations, e.g. |
|
@tomjn think I got through all of your notes, and did some more refactoring (namespace was bugging me). |
|
Looks good, my only change would have been to move the top level class to its own file and pass in the |
Fix for #5
@tomjn - let me know what you think.