Should I remove static function from my code?

Question

My code is located here: https://github.com/maniator/SmallFry

Should I make it so that that the App class does not have to use static functions but at the same time be able to set and set variables for the app from anywhere?

Or should I keep it how it is now with App::get and App::set methods?

What are the advantages and disadvantages of both?
How would I accomplish that 1st task if I was to undertake it?

Related Question

Sample code:

//DEFAULT TEMPLATE
App::set('APP_NAME', 'SmallVC');
//END DEFAULT TEMPLAT
//
//DEFAULT TEMPLATE
App::set('DEFAULT_TEMPLATE', 'default');
//END DEFAULT TEMPLATE
//DEFAULT TITLE
App::set('DEFAULT_TITLE', 'Small-VC');
//END DEFAULT TITLE
//LOGIN SEED
App::set('LOGIN_SEED', "lijfg98u5;jfd7hyf");
//END LOGIN SEED
App::set('DEFAULT_CONTROLLER', 'AppController');

---

 if(App::get('view')){
 $template_file = $cwd.'/../view/'.App::get('view').'/'.App::get('method').'.stp';
 if(is_file($template_file)){
 include $template_file;
 }
 else {
 include $cwd.'/../view/missingview.stp'; //no such view error
 }
 }
 else {
 App::set('template', 'blank');
 include $cwd.'/../view/missingfunction.stp'; //no such function error
 }

• hakre: You should at least give a small code example where you make use of the static functions you would like to remove. If you pass your $app object around, everywhere it is, you can access it, e.g. to get and set variables. Then you don't need to have global (static) functions or variables. Is that what you're looking for?
• Naftali: @hakre the code sample is in the related question and in the GIT
• hakre: Well replace the static function calls with non-static ones. In your controller's constructor, create an instance of App and assign it to a private member. Inside all functions of AppController, you can access it then. If other classes belong to the same "app", they need access to it as well, so you might want to offer a public getter function for it in AppController. The other alternative is, that the constructor requires to get an App object as a parameter (instead of instantiating it inside the controller), that's much better.
• Yoshi: Maybe a helpful read: Inversion of Control Containers and the Dependency Injection pattern and Symfony Dependency Injection
• netcoder: "not have to use static functions but at the same time be able to set and set variables for the app from anywhere". It's not possible, it's one or the other. The question you need to ask yourself is: "why do I need to access these variables from anywhere?". It usually demonstrates a design issue.

Answer 1

I think you have a feeling that static is bad. What I am posting may seem fairly crazy as it is a massive change. At the very least hopefully it presents a different idea of the world.

Miško Hevery wrote static methods are a death to testability.

I like testing, so for that reason I don't use them. So, how else can we solve the problem? I like to solve it using what I think is a type of dependency injection. Martin Fowler has a good but complicated article on it here.

For each object at construction I pass the objects that are required for them to operate. From your code I would make AppController become:

class AppController
{
 protected $setup;
 public function __construct(array $setup = array())
 {
 $setup += array('App' => NULL, 'Database' => NULL);
 if (!$setup['App'] instanceof App)
 {
 if (NULL !== $setup['App'])
 {
 throw new InvalidArgumentException('Not an App.');
 }
 $setup['App'] = new App();
 }
 // Same for Database.
 // Avoid doing any more in the constructor if possible.
 $this->setup = $setup;
 }
 public function otherFunction()
 {
 echo $this->setup['App']->get('view');
 }
}

The dependancies default to values that are most likely (your default constructions in the if statements). So, normally you don't need to pass a setup. However, when you are testing or want different functionality you can pass in mocks or different classes (that derive from the right base class). You can use interfaces as an option too.

Edit The more pure form of dependency injection involves further change. It requires that you pass always pass required objects rather than letting the class default one when the object isn't passed. I have been through a similar change in my codebase of +20K LOC. Having implemented it, I see many benefits to going the whole way. Objects encapsulation is greatly improved. It makes you feel like you have real objects rather than every bit of code relying on something else.

Throwing exceptions when you don't inject all of the dependencies causes you to fix things quickly. With a good system wide exception handler set with set_exception_handler in some bootstrap code you will easily see your exceptions and can fix each one quickly. The code then becomes simpler in the AppController with the check in the constructor becoming:

 if (!$setup['App'] instanceof App)
 {
 throw new InvalidArgumentException('Not an App.');
 }

With every class you then write all objects would be constructed upon initialisation. Also, with each construction of an object you would pass down the dependencies that are required (or let the default ones you provide) be instantiated. (You will notice when you forget to do this because you will have to rewrite your code to take out dependencies before you can test it.)

It seems like a lot of work, but the classes reflect the real world closer and testing becomes a breeze. You can also see the dependencies you have in your code easily in the constructor.

• Naftali: I mean I guess instead of $this->setup = $setup I could even do $this->App = $setup['App'] and $this->Database = $setup['Database']
• Naftali: This looks like a great answer all in all. Might need some major rewrites. Where would i create the App object? I start using App right way over here: github.com/maniator/SmallFry/blob/master/config/app_config.php
• hakre: Array as form of dependency injection container.
• Paul: @Neal Glad you like it, I was a bit worried it was too much change. For that its hard for me to tell. Would they be the default values for your App object in the AppController class? Now the thinking becomes constructing the entire object at initialisation, so if you are creating it from AppController you should probably know exactly how you want it setup? This may also lead you to doing less in your constructor.
• Jimbo: @Paul Can I ask why you don't type-hint 'App' in the constructor, and instead check in the constructor instead? Is there an architectural reason for this?

Answer 2

Well, if it was me, I would have the end goal of injecting the App dependency into any class (or class tree) that needs it. That way in testing or reusing the code you can inject whatever you want.

Note I said reuse there. That's because it's hard to re-use code that has static calls in it. That's because it's tied to the global state so you can't really "change" the state for a subrequest (or whatever you want to do).

Now, on to the question at hand. It appears that you have a legacy codebase, which will complicate things. The way I would approach it is as follows:

1. Create a non-static version of the app class (name it something different for now) that does nothing but proxy its get/set calls to the real app class. So, for example:

   class AppProxy {
    public function set($value) {
    return App::set($value);
    }
   }
   

   For now, all it has to do is proxy. Once we finish getting all the code talking to the proxy instead of the static app, we'll make it actually function. But until then, this will keep the application running. That way you can take your time implementing these steps and don't need to do it all in one big sweep.

2. Pick a main class (one that does a lot for the application, or is important) that you easily control the instantiation of. Preferably one that you instantiate in only one place (in the bootstrap is the easiest). Change that class to use Dependency Injection via the constructor to get the "appproxy".

   a. Test this!

3. Pick another class tree to work on, based on what you think will be most important and easiest.

   a. Test!!!

4. If you have more calls to App::, Go to #3

5. Change the existing App class to be non-static.

   a. Test!!!!!!!!!!

6. Remove the AppProxy and replace with App in the dependency injectors. If you did it right, you should only have one place to change to make this switch.

7. Pat yourself on the back and go get a drink, cause you're done.

The reason that I segmented it out like this is that once a step is completed (any step), you can still ship working software. So this conversion could take literally months (depending on the size of your codebase) without interrupting business as usual...

Now, once you're done, you do get some significant benefits:

1. Easy to test since you can just create a new App object to inject (or mock it as needed).

2. Side effects are easier to see since the App object is required wherever it could be changed.

3. It's easier to componentize libraries this way since their side effects are localized/

4. It's easier to override (polymorphism) the core app class if it's injected than if it's static.

I could go on, but I think it's pretty easy to find resources on why statics are generally bad. So that's the approach I would use to migrate away from a static class to an instance...

Answer 3

If you don't want to have static functions but global access from everywhere WITHOUT passing the object to the places where it is actually needed then you pretty much can only use one thing:

A global variable

So you are not really better of doing that. But that is the only thing i can think of that would fulfill your requirements.

If you App object is something like an application config a first possible step would be to pass it to the objects that need it:

class Login {
 public function __construct() {
 $this->_login_seed = App::get('LOGIN_SEED');
 self::$_ms = Database::getConnection();
 }

changes into:

class Login {
 public function __construct(App $app) {
 $this->_login_seed = $app->get('LOGIN_SEED');
 self::$_ms = Database::getConnection();
 }

• Naftali: see my updated question (sorry for the confusion). It seems like I want to use static, but i have been told from all sides not to
• edorian: In very short term (because that kinda goes beyond the scope of this question): You want no statics because global access to everything is usually a bad thing. you can't exchange classes because you hardwire everything together. If you want to get the benefits of OOP you need real objects. You shouldn't need global access to everything from everywhere but (generally speaking again) pass all the needed objects to the classes when you construct them.