6

I have been writing a PHP class that is exactly 450 lines long and it contains 14 static methods and 4 static properties as well as 6 constants (and private __construct() and __clone()).

I am wondering here is that am I doing something wrong, is my class evil?

When you use the class, you always call a single method like:

MyClass::coolMethod();

and then you leave it alone altogether so it feels that it would be stupid to make it constructable?

There's really not much point in constructing objects out of it, because it is more like a tool that contains a few methods that you can just call directly.

Actually, out of those 14 methods, 7 of them are public -- the rest are private for the class to use.

James A Mohler
  • 11,060
  • 15
  • 46
  • 72
Tower
  • 98,741
  • 129
  • 357
  • 507

5 Answers5

4

You should avoid static as much as global.

Statics give you the same disadvantages globals give you. Whenever you are using any class methods, you are hardcoding a dependency on that class into the consuming code. The result is less maintainable tightly coupled code. This can easily be avoided by avoiding statics altogether and a disciplined use of Dependency Injection.

You cannot inject and pass around static classes, so for instance when you have to unit-test them, you cannot mock them (or at least only with some effort). It's just plain painful. Static methods are death to testability.

Also, keep in mind that classes should do only one thing. They should have a single responsibility. Go through your class to see if there is stuff in there that's better placed somewhere else to avoid writing a God Class.

Gordon
  • 312,688
  • 75
  • 539
  • 559
3

It depends on the purpose of this class. If the methods are mostly incoherent in terms of data, this is a perfectly valid solution of grouping functions (now methods). This is a very bad idea if you need to share values between functions, since that would be more than a simple list of functions, grouped under a common name. Namespaces are another option, but if you're using a PHP-version lower than 5.3, this is probably the best solution.

jwueller
  • 30,582
  • 4
  • 66
  • 70
  • 1
    Agreed - this also helps avoid the classic "include(crapload_of_functions.php)" that many php frameworks/applications often use – JayTee Dec 06 '10 at 15:10
  • 1
    It helps avoiding that only if you use autoloading. By grouping them in classes you make your code just cleaner, not faster. – Alin Purcaru Dec 06 '10 at 15:13
2

This is like saying, "I have a house with four bedrooms. Is that bad?"

Static methods are neither good nor bad. Having fourteen methods is neither good nor bad. Having fourteen static methods is, by extension, neither good nor bad.

If in your fourteen methods you're going to great lengths to simulate object instances, or to simulate inheritance, then something has gone horribly wrong. PHP will let you create instances, and supports inheritance, so it would be silly to try to simulate them any other way.

But if you're just using your class essentially like a namespace, where the functions and data all work together but there are no individual instances of the class to contend with, there's absolutely nothing wrong with that.

VoteyDisciple
  • 37,319
  • 5
  • 97
  • 97
1

Not bad. However, with all those static props, you might want to consider making this a singleton.

Here is some singleton code I am using in the framework I am building. You can tear it apart and make it the sole public method for your class that returns the one version of itself.

class ClassName {
    function getInstance()
        {
            static $instance;

            if (!isset($instance)) 
      {
                $instance = new ClassName();
            }

            return $instance;
        }
}

You would use this, then, by doing ClassName::GetInstance()->othermethod();

The class can then have tons of private values and otherwise gain all the nice things you have with an object.

DampeS8N
  • 3,621
  • 17
  • 20
  • I think singletons are a very bad idea (they're one of the most popular examples for anti-patterns). If he really needs private values, then he needs a real class for that purpose. He just wants to group some functions, like in a namespace, as far as i understand. – jwueller Dec 06 '10 at 15:21
  • I'm not sure singletons are anti-patterns. They absolutely can be misused, but that is true of all patterns. What leads you to call them an anti-pattern? - I should mention that in this framework, the lone singleton is the core application that needs to not only track a lot of data in one place, but also make available a lot of communal resources (like the models). Any other method would require passing around references to that control object. And we all know how loopy php and references can be. – DampeS8N Dec 06 '10 at 15:26
  • @DampeS8N: A singleton introduces global state, which is bad. Additionally, they make testing a real mess. The service locator pattern would be a good alternative to passing references like crazy. – jwueller Dec 06 '10 at 15:36
  • Singletons are anti-patterns, especially in PHP where everything just lives for the request anyway. We dont have application servers where instances are shared in memory between requests like Java does. – Gordon Dec 06 '10 at 15:37
  • Global state is only bad if it is untrackable. Global state is, by definition, what we are talking about here. And, besides... I may be misremembering, but isn't Service Locator an example of Singleton? http://java.sun.com/blueprints/patterns/ServiceLocator.html – DampeS8N Dec 06 '10 at 15:41
  • @Gordon: Now here is something I can agree with. The term 'singleton' is not accurate under PHP. However, the pattern itself, an object that for one request is the lone object through-which all requests of a type are routed, is sound. – DampeS8N Dec 06 '10 at 15:43
  • @Dampe Why does it have to be a singleton for that purpose? Just do not instantiate more than one instance. Then use Dependency Injection to get the instance to where it is needed. – Gordon Dec 06 '10 at 17:17
  • That requires trust. Nothing would then prevent someone from misusing the class and instantiating new instances. Seems control over usage of code is the whole point of 99% of OOP. Not making something that should have only one concurrent instance, only be capable of one instance, would be a weakness. – DampeS8N Dec 06 '10 at 17:23
0

I would say that no, it isn't bad. In fact that was the only way to fake certain behavior before. It was for instance a way to fake namespaces. One could encapsulate functions in static classes instead of having them "out in the free". So allot of php developers are familiar with this and it won't actually confuse most people. What you SHOULD try to do nowadays though is to make use of PHP's "new" namespace feature and if needed combine it with a singleton pattern if you actually need to store data in an object format. You could just as well have a "global" variable contained in your namespace and that could work fine at times to. But take a look at namespaces and see if that fits you in any way and after that see if singleton pattern might match your specific needs.

inquam
  • 12,664
  • 15
  • 61
  • 101