I was introduced to a code the other day, that had a bug. It was pretty easy to reproduce, and I could easily create a Selenium test case for the problem. I was told, that the code is a mess and I'm free to do whatever I want.
When you're doing refactoring, you should always know what exactly you are doing. In this case the goal was to eliminate the bug causing the security issues. The ~500LOC method had an enormous cyclometric complexity, and more static calls than I've ever written. At least it was in some sort of a class, but not a single comment were there to guide me through the tough logic that merges security settings from various extensions (modules) and calculates the dependencies and verifies values and draws forms and ... hard-to-test code heaven...
So what to do when you see a
[cc lines="-1" lang="php" escaped="true"]
class DemoClass
{
public function someMethod()
{
$val1 = SomeClass1::staticMethod1();
$val2 = SomeClass2::staticMethod2();
}
public function process()
{
if ($_SERVER['REQUEST_METHOD'] === 'POST' && isset($_POST['fieldValue'])) {
return $this->_doSomethingWith($_POST['fieldValue']);
}
return null;
}
private function _doSomethingWith($field)
{
return strrev( (string) $field);
}
}[/cc]
With a client code:
[cc lines="-1" lang="php" escaped="true" theme="blackboard"]
$demo = new DemoClass();
$demo->someMethod();
print $demo->process();
[/cc]
If refactoring the statics is not yet an option,
use wrappers!
[cc lines="-1" lang="php" escaped="true"]
class SomeClass1Wrapper
{
public function staticMethod1()
{
return SomeClass1::staticMethod1();
}
}
class SomeClass2Wrapper
{
public function staticMethod2()
{
return SomeClass2::staticMethod2();
}
}
[/cc]
Then you can easily add the dependencies for your method
[cc lines="-1" lang="php" escaped="true"]
class DemoClass
{
public function someMethod(
SomeClass1Wrapper $someClass1,
SomeClass2Wrapper $someClass2
) {
$val1 = $someClass1->staticMethod1();
$val2 = $someClass2->staticMethod2();
}
public function process()
{
if ($_SERVER['REQUEST_METHOD'] === 'POST' && isset($_POST['fieldValue'])) {
return $this->_doSomethingWith($_POST['fieldValue']);
}
return null;
}
private function _doSomethingWith($field)
{
return strrev( (string) $field);
}
}
[/cc]
And there you go. You have the statics in control.
The client code needs to be modified:
[cc lines="-1" lang="php" escaped="true" theme="blackboard"]
$demo = new DemoClass();
$someClass1 = new SomeClass1Wrapper();
$someClass2 = new SomeClass2Wrapper();
$demo->someMethod($someClass1, $someClass2);
print $demo->process();
[/cc]
The other thing I've bumped into is using [cci_php]$_POST[/cci_php] and [cci_php]$_SERVER[/cci_php] in the methods.
Doing this you won't have such a hard time testing, but you'll hopefully won't want to set variables in these arrays by hand to reproduce some scenarios.
What's wrong with globals?
Accessing global state statically doesn’t clarify those shared dependencies to readers of the constructors and methods that use the Global State. Global State and Singletons make APIs lie about their true dependencies. To really understand the dependencies, developers must read every line of code. It causes Spooky Action at a Distance: when running test suites, global state mutated in one test can cause a subsequent or parallel test to fail unexpectedly.
Misko Hevery: Flaw: Brittle Global State & Singletons
So what can you do? Give them to your method. Or if they're used widely in your code, pass them in the constructor. (you do have one do you?)
When I realized I need all dependencies throughout the whole class, I've moved them up one level.
[cc lines="-1" lang="php" escaped="true"]
class DemoClass
{
/**
* @var SomeClass1Wrapper
*/
private $_someClass1;
/**
* @var SomeClass2Wrapper
*/
private $_someClass2;
/**
* @var array
*/
private $_serverVars=array();
/**
* @var array
*/
private $_postVars=array();
public function __construct(
SomeClass1Wrapper $someClass1,
SomeClass2Wrapper $someClass2,
array $serverVars=array(),
array $postVars=array()
) {
$this->_someClass1 = $someClass1;
$this->_someClass2 = $someClass2;
$this->_serverVars = $serverVars;
$this->_postVars = $postVars;
}
public function someMethod()
{
$val1 = $this->_someClass1->staticMethod1();
$val2 = $this->_someClass2->staticMethod2();
}
public function process()
{
if ($this->_serverVars['REQUEST_METHOD'] === 'POST' && isset($this->_postVars['fieldValue'])) {
return $this->_doSomethingWith($this->_postVars['fieldValue']);
}
return null;
}
private function _doSomethingWith($field)
{
return strrev( (string) $field);
}
}
[/cc]
Then the client code wil be:
[cc lines="-1" lang="php" escaped="true" theme="blackboard"]
$someClass1 = new SomeClass1Wrapper();
$someClass2 = new SomeClass2Wrapper();
$demo = new DemoClass($someClass1, $someClass2,$_SERVER,$_POST);
$demo->someMethod();
print $demo->process();
[/cc]
The other thing you should look for in a code is the usage of the variables. For example, when I've searched for "where do I use the $this->_serverVars", I've found the following:
[cc lines="-1" lang="php" escaped="true"]
public function process()
{
if ($this->_serverVars['REQUEST_METHOD'] === 'POST' && isset($this->_postVars['fieldValue'])) {
return $this->doSomethingWith($this->_postVars['fieldValue']);
}
... //some lines of other stuff
if ($this->_serverVars['REQUEST_METHOD'] === 'POST') {
$this->doSomethingElse();
}
... //some lines of other stuff
if ($this->_serverVars['REQUEST_METHOD'] === 'POST') {
$this->doSomethingNaughty(true);
}
return null;
}
[/cc]
You should note, that the code is only using the 'REQUEST_METHOD' element, nothing else.
Tell, Don't Ask
The "Tell, Don't Ask" principle suggests that you should tell objects what to do rather than ask for their state (i.e., their data), make a decision, and then tell them what to do.
The guiding idea behind this is that an object should avoid exposing it's internal state to another object. Consider a quote from The Art of Enbugging which discusses the story of "The Paperboy and the Wallet".
Suppose the paperboy comes to your door, demanding payment for the week. You turn around, and the paperboy pulls your wallet out of your back pocket, takes the two bucks, and puts the wallet back. Instead of asking for the wallet, the paperboy should tell the customer to pay the $2.00.
from & more @ http://learn.objectorientedcoldfusion.org/wiki/Tell_Dont_Ask
So I've modified my code:
[cc lines="-1" lang="php" escaped="true"]
class DemoClass
{
/**
* @var SomeClass1Wrapper
*/
private $_someClass1;
/**
* @var SomeClass2Wrapper
*/
private $_someClass2;
/**
* @var string
*/
private $_requestMethod='GET';
/**
* @var array
*/
private $_postVars=array();
public function __construct(
SomeClass1Wrapper $someClass1,
SomeClass2Wrapper $someClass2,
$requestMethod='GET', //assuming
array $postVars=array()
) {
$this->_someClass1 = $someClass1;
$this->_someClass2 = $someClass2;
$this->_requestMethod = $requestMethod;
$this->_postVars = $postVars;
}
public function someMethod()
{
$val1 = $this->_someClass1->staticMethod1();
$val2 = $this->_someClass2->staticMethod2();
}
public function process()
{
if ($this->_requestMethod === 'POST' && isset($this->_postVars['fieldValue'])) {
return $this->_doSomethingWith($this->_postVars['fieldValue']);
}
return null;
}
private function _doSomethingWith($field)
{
return strrev( (string) $field);
}
}
[/cc]
Then the client code wil be:
[cc lines="-1" lang="php" escaped="true" theme="blackboard"]
$someClass1 = new SomeClass1Wrapper();
$someClass2 = new SomeClass2Wrapper();
$demo = new DemoClass($someClass1, $someClass2,$_SERVER['REQUEST_METHOD'],$_POST);
$demo->someMethod();
print $demo->process();
[/cc]
And there you have a nice, testable, mockable code. Here's the test file:
[cc lines="-1" lang="php" escaped="true"]
require_once 'PHPUnit/Framework.php';
require_once dirname(__FILE__).'/DemoClass.php';
class DemoClassTest extends PHPUnit_Framework_TestCase
{
/**
* @var DemoClass
*/
protected $object;
/**
* @var someClass1Wrapper
*/
protected $someClass1Mock;
/**
* @var someClass2Wrapper
*/
protected $someClass2Mock;
/**
* Sets up the fixture, for example, opens a network connection.
* This method is called before a test is executed.
*/
protected function setUp()
{
$this->someClass1Mock = $this->getMock(
'someClass1Wrapper',
array('staticMethod1'),
array(),
'',
false);
$this->someClass2Mock = $this->getMock(
'someClass2Wrapper',
array('staticMethod2'),
array(),
'',
false);
}
/**
* Test that the static methods are called
*/
public function testSomeMethod()
{
$this->object = new DemoClass($this->someClass1Mock, $this->someClass2Mock);
$this->someClass1Mock->expects($this->at(0))
->method('staticMethod1');
$this->someClass2Mock->expects($this->at(0))
->method('staticMethod2');
$this->object->someMethod();
}
/**
* Test that we get the expected null if the method is GET
*/
public function testProcessWithGet()
{
$this->object = new DemoClass($this->someClass1Mock, $this->someClass2Mock);
$expected = null;
$actual = $this->object->process();
}
/**
* Test that we get the expected null if there's no 'fieldValue'
*/
public function testProcessWithPostAndNoInput()
{
$this->object = new DemoClass($this->someClass1Mock, $this->someClass2Mock,'POST');
$expected = null;
$actual = $this->object->process();
}
/**
* Test that the process returns the reversed 'fieldValue'
*/
public function testProcessWithPostAndInput()
{
$input = 'Refactoring is fun';
$postVars = array('fieldValue'=>$input);
$this->object = new DemoClass(
$this->someClass1Mock,
$this->someClass2Mock,
'POST',
$postVars);
$expected = 'nuf si gnirotcafeR';
$actual = $this->object->process();
$this->assertEquals($expected, $actual);
}
}
[/cc]
Of course testing is only easy when your base objects' responsibilities are clear and simple. In my case, I still haven't found the cause of the bug, since the whole code is more complex and still has a lot of work to do. In the next chapter of hunting down the strange error I'll fill you in with my experiences of finding out what and how the original code does, before you can carry on with refactoring.
