[e_i_pi]

I've been programming in OOP for a while now, and have used static methods on several occasions. I've also read a lot of articles advising against using the static keyword.

I tend to use static methods in abstract classes that are designed using Singleton or Factory patterns, or where there are functions I want accessible within a namespace that has an abstract superparent class. Another reason I use them is sometimes I have instantiable classes, but don't need an entire object, just a particular method (particularly validation methods). An example would be my Project Model, which has dozens of methods covering a range of functionality, but sometimes all I need is a call to \MyNamespace\ProjectModel::IsProjectApproved().

I'm wondering what other people use static methods for, what bad usage is, what good usage is, whether to avoid them, and why?

This post has been edited by e_i_pi: 21 May 2012 - 05:28 PM

[AdaHacker]

I think the articles you linked to pretty well summed up the reasons not to use static methods. Of course, there's nothing wrong with static methods per se - assuming you understand what you're doing. They're just one technique among many, each with different trade-offs.

They only become problematic when people use them thoughtlessly or fool themselves into thinking that "object-oriented programming" just means putting all your functions in a class. For example, we had something like this at my last job. We had data objects with a bunch of getters and setters for properties (but nothing else), and then separate "model" classes that were entirely static. The thing is, hardly any of those "model" methods actually used the corresponding data object. Instead, they pretty much all took associative arrays. (For the record, all this was built before I was hired, so it was not my idea.) The general pattern looked something like this:

class Example {
    private $name;
    private $id;

    public function getId() { return $this->id; }
    public function setId($id) { $this->id = $id; }
    public function getName() { return $this->name; }
    public function setName($name) { $this->name = $name; }

}

class ExampleModel {
    public static function getExample($id) {
        /* Returns an Example object. */
    }
    public static function updateExample($id, $data) {
        /* Takes the row ID to update and an array of the form array('id'=>123, 'name'=>'Bob') for $data. */
    }
    public static function insertExample($data) {
        /* Takes an array of the form array('id'=>123, 'name'=>'Bob') for $data. */
    }
}

The big problem with this is that we were just sort of pretending to do object-oriented programming. In reality, it was procedural programming wrapped up in classes. Everything meaningful happened in static methods and the classes with instance methods were just sort of there to make things look object-oriented. In one discussion about it, I raised the point that we could just as well do away with the data classes, because the the only thing we ever used them for was reading the fields returned from the database. For everything else, we passed around arrays, so we could just drop the Example class switch to using arrays for everything with no loss of functionality. I mean, why bother kidding ourselves?

For my personal projects, these days I tend to avoid static methods when possible - usually just for things like singletons and factory methods, as you mentioned. This is in large part because I've been trying to move more toward TDD, and you can't do DI with static classes. When I do use them, I try to follow the general principle that static methods should only be used for cases where you know you'll never need to override or extend that functionality. Note that I say "try" because sometimes I just get lazy and care more about accomplishing something than about architectural purity. If the choice between a static method and a new class isn't likely to have architectural ramifications down the road, I tend not to worry about it too much. That's what refactoring is for.

e_i_pi, on 21 May 2012 - 08:27 PM, said:

Another reason I use them is sometimes I have instantiable classes, but don't need an entire object, just a particular method (particularly validation methods). An example would be my Project Model, which has dozens of methods covering a range of functionality, but sometimes all I need is a call to \MyNamespace\ProjectModel::IsProjectApproved().

That's an interesting example, because to me that smells like an anti-pattern. Why not use something like $project->isApproved()? After all, it seems like approval state would be a property individual to each project. Is it just out of pure laziness that you want to do it in one line rather than create the object and then check the property? While I can't condone that, I totally understand (and have been guilty of it myself). Or is this perhaps due to your choice of data access method, e.g. you're using an active record pattern that fetches on object creation, but you only need to fetch one column? If that's the case, the use of a static method is beside the point - it's just covering up the fact class should be using a different data access strategy.

[e_i_pi]

Thanks for the long post, it's good to read about real world examples. I'd agree that that use of objects is against the core principles of OOP since it avoids extension. Also agree in regards to the links I provided, though I provided those links to demonstrate that there's a lot of 'nays' for static methods, and not many 'yeas'.

In regards to my code, I'm also unsure of whether it is an anti-pattern or not, which I guess is one of the reasons I started this topic. I'll explain what my code does to get to the stage where it calls the method statically.

Several methods in my code (almost all actually) make design-by-contract checks, using a method we call, for arguments sake, TestCondition. This method lies in the topmost class of the application, as it is a framework method, and needs to be accessible by any extending class and hence object.

Let's say, for example, the user wants to edit an attachment to a project they are working on. When they submit the form, and the application calls the appropriate method, the header of the called method would look something like this:

$this->TestCondition(array(self::SECURITY_CHECK_USER_IS_PROJECT_OWNER, $projectId, $userId));
$this->TestCondition(array(self::SECURITY_CHECK_PROJECT_IS_EDITABLE, $projectId));
$this->TestCondition(array(self::SECURITY_CHECK_ATTACHMENT_BELONGS_TO_PROJECT, $attachmentId, $projectId));

All those references to self::SECURITY_CHECK_* are constants declared in the topmost class as well. Now, the TestCondition() method looks something like this:

final protected function TestCondition(Array $ary = array())
{
	if($ary === array())            throw new Exception('No array given for call to ' . __METHOD__);
	switch($ary[0])
	{
		// <snip>
		case self::SECURITY_CHECK_PROJECT_IS_EDITABLE:
			return ProjectModel::IsProjectEditable(array('ProjectID' => (int)$ary[1]));
			break;
		case self::SECURITY_CHECK_USER_IS_PROJECT_OWNER:
			self::SetDefaultArrayKeyValue($ary, 2, User::ID());
			return ProjectModel::IsProjectOwner(array('ProjectID' => $ary[1], 'UserID' => (int)$ary[2]));
			break;
		case self::SECURITY_CHECK_ATTACHMENT_BELONGS_TO_PROJECT:
			return ProjectModel::IsProjectAttachment(array('AttachmentID' => $ary[1], 'ProjectID' => (int)$ary[2]));
			break;
		// <snip>
	}
}

If I were to change it so that each case statement instantiates an object of type ProjectModel, then I'd have 3 instantiated objects just to do the security checks for one method. To me, that seems like a waste of resources. This is why I have declared those methods as static.

I can't see a way to have just one ProjectModel object, since any method within the application may call multiple TestCondition methods, each with different parameters (read: different Projects, i.e. different properties). Is it an anti-pattern or not? I'm not sure, but it's the path I've taken.

[Atli]

That example, e_i_pi, doesn't make much sense to me. Why are you doing those checks outside of the method that is actually editing the project attachment? Why do you define what amounts to a global function to do those checks before calling the edit on the ProjectModel?

I've never used that pattern before, so I may well be missing something, but it seems that it would be better to do those tests inside whichever method does the edit, and have it throw exceptions when they fail. (This should be good for TDD as well.)

I'd picture that somewhat like:

<?php
interface ProjectSecurityCheck {
	public function IsEditable();
	public function IsOnwer($userID);
	public function IsAttachment($attID);
}

<?php
class ProjectModel implements ProjectSecurityCheck {
	public function IsEditable() {
		return $something;
	}
	
	public function IsOwner($userId) {
		return $something;
	}

	public function IsAttachment($attID) {
		return $something;
	}

	public function EditAttachment($attID, $userID) {
		if (!$project->IsEditable() {
			throw new Exception("Project isn't editable");
		}
		else if (!$project->IsOwner()) {
			throw new Exception("Only the owner can edit a project")
		}
		else if (!$project->IsAttachment($attID)) {
			throw new Exception("The given attachment is not attached to this project.");
		}
		else {
			// Edit stuff...
		}
	}
}

<?php
try {
	$project = new ProjectModel($projectID);
	$project->EditAttachment($attID, $userID);
}
catch (Exception $e) {
	Logger::add($e);
}

[e_i_pi]

Hmm, I think you're right Atli and AdaHacker. I was worried that the pattern I was using was bunk. Sigh, back to the drawing board again lol.

With my class contracting, there's three general categories of contracts that I use - verification, security checks, and validation. The way these are handled when they fire is:

• Failed verification checks points towards a critical problem in the code. An Exception is thrown, code halts immediately, no nice message is given to the user.
  
• Failed security checks suggest either malicious intent by the user, or the state of the calling object has changed such that the user can no longer perform that action. The admin is notified via email, and a nice message is given to the user.
  
• Failed validation checks mean that the code can no longer execute as planned, but there was no known malicious intent. The admin is notified via logs, and a nice message is given to the user.

Here is an example of these contractual check types as class constants:

/** @var int Check for the existence of at least one array key.<br>
 * (::Code, array($key), $array) */
const VERIFY_AT_LEAST_ONE_ARRAYKEY = 1002;

/** @var int Check that a project is editable.<br>
 * (::Code, DB:Projects.ID) */
const SECURITY_CHECK_PROJECT_IS_EDITABLE = 2002;

/** @var int Check that an ID is null or exists for a particular DB table.<br>
 * (::Code, ::TableName, ID) */
const VALIDATE_NULLABLE_DBID = 3002;

It seems that Verification and Validation checks are suitable to be in the topmost class (since they could apply to any type of object), but the Security check is an object-specific check, so should reside in the class itself.