I have a friend who recently started a new programming job and he kept complaining that his code was being rejected. Most of the time it was because his methods and classes were too long; it didn’t make sense to him why he needed more classes or methods to accomplish something simple. He felt that it would make his code more complex and harder to debug instead of just keeping it only one class and a couple of methods at most.
This is an advice that we programmers tend to give a lot, but most of the time we don’t really explain why is a good advice. Some people take it to far and I see why you would think a class with too many methods will be more complicated than one with very few. In the interest of keeping things simple, I won’t go into the details of SRP ( Single Responsibility Principle ) and encapsulation, but a basic definition would be that a class or method should be responsible for one thing and you can change it, without affecting the other parts of the system.
Let’s say that we need to send an email with some data when a user is older than 18. The data will be read from a config file. To keep it short we’ll assume that the email sender and config are in different classes.
class MemberEmail
{
const MAX_AGE = 18;
public function sendEmailToMember( $user )
{
if ( $user->age > self::MAX_AGE )
{
$config = new SomeConfig( );
$values = $config->getValues( );
if ( empty( $values ) === false )
{
$emailSender = new EmailSender( );
foreach ( $values as $value )
{
$emailSender->sendEmail( $value );
}
return true;
}
}
return false;
}
}
Seems simple and easy to read, but if you take a closer look, its not very testable since we can only test on user age and not if the config is empty.
class MemberEmailTest
{
protected $memberEmail;
public function setUp( )
{
$this->memberEmail = new MemberEmail( );
}
public function testSendEmailToMemberReturnsFalse( )
{
// Test user is younger than 18
$user = new User( );
$user->age = 16;
$this->assertFalse( $this->memberEmail->sendEmailToMember( $user ) );
// Test config is empty. Wait how can I mock SomeConfig?
$user->age = 18;
}
}
So we need to refactor the class by taking out the read config logic into its own method.
public function getValuesFromConfig( )
{
$config = SomeConfig( );
return $config->getValues( );
}
By doing this, we now can test both conditions by stubbing the getValuesFromConfig method and defining our own value. This allows us to check that the config is returned as an array as well.
public function testSendEmailToMemberReturnsFalse( )
{
// Test user is younger than 18
$user = new User( );
$user->age = 16;
$this->assertFalse( $this->memberEmail->sendEmailToMember( $user ) );
//Test config is empty
$user->age = 18;
$stub = $this->getMockBuilder( 'MemberEmail' )
->setMethods( array('getValuesFromConfig') )
->getMock( );
$stub->expects( $this->any( ) )
->method( 'getValuesFromConfig' )
->will( $this->returnValue( array( ) ) );
$this->assertFalse( $this->memberEmail->sendEmailToMember( $user ) );
}
But even though this improves things, the class is still not very maintainable. What if the action now depends on the user being a certain age, gender, or country? We could create a lot of methods to validate each thing, but our class would do too much. Don’t forget SRP, so lets create another class to handle this.
class MemberEmailValidator
{
const MAX_AGE = 18;
const VALID_GENDER = 'female';
const VALID_COUNTRY = 'Mexico';
public function userCanSendEmail( $user )
{
$this->validateAge( $user->age );
$this->validateGender( $user->gender );
}
protected function validateAge( $age )
{
if ( $age < self::MAX_AGE )
{
$this->raiseException( 'Too young' );
}
}
protected function validateGender( $gender )
{
if ( $gender !== self::VALID_GENDER )
{
$this->raiseException( 'wrong gender' );
}
}
protected function raiseException( $msg )
{
throw new Exception( $msg );
}
}
Now we can write separate tests as well and know that if we change our validations we will not affect the other class and viceversa.
class MemberEmailValidatorTest
{
protected $myValidator;
public function setUp( )
{
$this->myValidator = new MemberEmailValidator( );
}
/**
* @expectedException Exception
*/
public function testValidateAgeThrowsException( )
{
$this->myValidator->validateAge( 12 );
}
/**
* @expectedException Exception
*/
public function testValidateGenderThrowsException( )
{
$this->myValidator->validateGender( 'male' );
}
}
Quick note, when writing these kind of validators, I like to use exceptions in the private methods instead of returning true or false. That way I don’t have to write one big if and I can take advantage of the @expectException notation. This leads to cleaner tests.
Now the code is looking a lot better.
class MemberEmail
{
public function sendEmailToMember( $user )
{
$myValidator = new MemberEmailValidator( );
try
{
$myValidator->userCanSendEmail( $user );
$values = $this->getValuesFromConfig( );
if ( empty( $values ) === false )
{
$emailSender = new EmailSender( );
foreach ( $values as $value )
{
$emailSender->sendEmail( $value );
}
return true;
}
}
catch( Exception $e )
{
// fail silently or log this
}
return false;
}
public function getValuesFromConfig( )
{
$config = SomeConfig( );
return $config->getValues( );
}
}
But wait! There’s more. We still have not completely solved our testable problem. We moved one dependency into its own method but we still have two left that will cause problems. We can create new methods for them, but remember when I told you that some people take it too far with creating a lot of methods?
Dependency Injection (DI) to the rescue!
Instead of the class getting the object it needs, you provide the class with the other objects they’ll use by “injecting” it, just like the name says. One of the advantages of using DI is that it makes our tests a lot easier, instead of mocking our config object like we did above, we can create an empty config object and use that for out tests. For this, always prefer interfaces over direct classes, it will make your code cleaner and easier to maintain since no matter what object is passed along you’ll know that it will have the methods it needs.
interface IConfig
{
public function getValues( );
}
class SomeConfig implements IConfig
{
public function getValues( )
{
// code for retrieving values
}
}
class EmptyConfig implements IConfig
{
public function getValues( )
{
return array( );
}
}
Dependencies can be injected in the constructor or with setter methods, lets use the constructor
class MemberEmail
{
protected $myValidator;
protected $emailSender;
protected $someConfig;
public function __construct( IValidator $myValidator = null, IEmailSender $emailSnder = null, IConfig $someConfig = null )
{
$this->myValidator = ( $myValidator === null ) ? new MemberEmailValidator( ) : $myValidator;
$this->emailSender = ( $emailSender === null ) ? new EmailSender( ) : $otherClass;
$this->someConfig = ( $someConfig === null ) ? new SomeConfig( ) : $someConfig;
}
public function sendEmailToMember( $user )
{
try
{
$this->myValidator->userCanSendEmail( $user );
$values = $this->getValuesFromConfig( );
if ( empty( $values ) === false )
{
foreach( $values as $value )
{
$this->emailSender->sendEmail( $value );
}
return true;
}
}
catch( Exception $e )
{
//Log or fail silently
}
return false;
}
public function getValuesFromConfig( )
{
return $this->someConfig->getValues( );
}
}
Now we can write our tests and be sure we won’t be using production data
class MemberEmailTest
{
protected $emailSender;
public function setUp( )
{
$this->emailSender = new EmailSenderStubbed( );
}
public function testDoSomethingReturnsFalseNoConfig( )
{
$user = new User( );
$user->age = 18;
$user->gender = 'female';
$config = new EmptyConfig( );
$myValidator = new MemberEmailValidator( );
$memberEmail = new MemberEmail( $myValidator, $this->emailSender, $config );
$this->assertFalse( $memberEmail->sendEmailToMember( $user ) );
}
}
So in the end we took 20 lines of code and turned them into 67 lines ( 2 classes, not counting interfaces ), but I hope that you see that by taking “simple” classes and breaking them into smaller ones, you end up with code that’s flexible to change, easier to debug, and cleaner.
