Qafoo GmbH - passion for software quality
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
:Author: Kore Nordmann
:Date: Tue, 03 May 2016 08:58:13 +0200
:Revision: 6
:Copyright: All rights reserved
==================
Never Use ``null``
==================
:Abstract:
When doing code reviews together with our customers we see a pattern
regularly which I consider problematic in multiple regards – the usage of
``null`` as a valid property or return value. We can do better than this.
:Description:
When doing code reviews together with our customers we see a pattern
regularly which I consider problematic in multiple regards – the usage of
``null`` as a valid property or return value. We can do better than this.
:Keywords:
clean code, null, object oriented design
When doing code reviews together with our customers we see a pattern regularly
which I consider problematic in multiple regards – the usage of ``null`` as a
valid property or return value. We can do better than this.
Let's go into common use cases first and then discuss how we can improve the
code to make it more resilient against errors and make it simpler to use. Most
issues highlighted are especially problematic when others are using your source
code. As long as you are the only user (which hopefully is not the case) those
patterns might be fine.
What it is Used For
===================
One common use case is setter injection for optional dependencies, like::
class SomeLoggingService {
private $logger = null;
public function setLogger(Logger $logger) {
$this->logger = $logger;
}
}
The logger will be set in most cases but somebody will forget this when using
your service. Now a second developer enters the scene and writes a new method
in this class using the property ``$logger``. The property is always set in the
use cases they test during development, thus they forget to check for ``null``
– obviously this will be problematic in other circumstances. You rely on
methods called in a certain order which is really hard to document. An internal
``getLogger()`` method constructing a default null logger might solve this
problem, but it still might not be used because the second developer wasn't
aware of this method and just used the property.
In PHP versions < 7 a call to ``$this->logger->notice(…)`` will result in a
``Fatal Error`` which is particularly bad since the application can't handle
this kind of errors in a sane way. In PHP 7 those errors are finally catchable
but still nothing you'd expect in this situation.
What is even worse is *debugging* this kind of initialization. This is often
even used together with aggregated objects which are *required* by the
aggregating class. (You should not use setter injection for mandatory
aggregates, but it is still used this way.) Let's consider the following code
now::
class SomeService {
public function someMethod() {
$this->mandatoryAggregate->someOtherMethod(/* … */);
}
}
When calling ``someMethod()`` and the property ``$mandatoryAggregate`` is not
initialized we get a fatal error, as mentioned. Even if we get a backtrace
through XDebug__ or change the code to throw an exception and get a backtrace
it is still **really hard** to understand why this property is not initialized
since the construction of ``SomeService`` usually happens outside of the
current callstack but inside the Dependency Injection Container or during
application initialization.
The debugging developer is now left with finding all occurrences where
``SomeService`` is constructed, check if the ``$mandatoryAggregate`` is
properly initialized and fix it, if not.
__ https://xdebug.org/
The solution
------------
All mandatory aggregates **must always** be initialized during construction. If
you want a slim constructor consider a pattern like the following::
class SomeService {
public function __construct(Aggregate $aggregate, Logger $logger = null) {
$this->aggregate = $aggregate;
$this->logger = $logger ?: new Logger\NullLogger();
}
}
The parameter ``$aggregate`` now is really mandatory, while the logger is
optional – but it will still always be initialized. The ``Logger\NullLogger``
now can be logger which just throws all log messages away. This way there is no
need to care about checking the logger every time you want to use it.
Use a so called null object if you need a default instance which does nothing.
Other examples for this could be a null-mailer (not sending mails) or a
null-cache (does not cache). Those null objects are usually really trivial to
implement. Even it costs time to implement those you'll safe a lot time in the
long run because you will not run in ``Fatal Errors`` and have to debug them.
``null`` as Return Value
========================
A similar situation is the usage of ``null`` as a return value for methods
which are documented to return something else. It is still commonly used in
error conditions instead of throwing an exception.
It is, again, a lot harder to debug if this occurs in a software you use but
you are not entirely familiar with. The ``null`` return might pass through
multiple call layers until it reaches your code which makes debugging that kind
of code a journey through layers of foreign and undiscovered code – sometimes
this can be fun but almost never what you want to do when in a hurry::
class BrokenInnerClass {
public function innerMethod() {
// …
if ($error) {
return null;
}
// …
}
}
class DispatchingClass {
public function dispatchingMethod() {
return $this->brokenInnerClass->innerMethod();
}
}
class MyUsingClass {
public function myBeautifulMethod() {
$value = $this->dispatchingClass->dispatchingMethod();
$value->getSomeProperty(); // Fatal Error
}
}
Usually there are even more levels of indirection, of course. We live in the
age of frameworks after all.
The solution
------------
If a value could not be found do not return ``null`` but throw an exception –
there are even built in exceptions for such cases like the
``OutOfBoundsException``, for example.
In the callstack I can see immediately where something fails. In the optimal
case the exception message even adds meaning and gives some hints of what I
have to fix.
Summary
=======
Using ``null`` can be valid inside of value objects and sometimes you just want
to show nothing is there. In most cases ``null`` should be either replaced by
throwing an exception or providing a null object which fulfills the API but
does nothing. Those null objects are trivial and fast to develop. The return on
investment will be **huge** due to saved debugging hours.
..
Local Variables:
mode: rst
fill-column: 79
End:
vim: et syn=rst tw=79
Trackbacks
==========
Comments
========
- Richard at Tue, 03 May 2016 11:19:26 +0200
In your first example. It sounds like $logger is required. It should be
passed as part of the constructor so the coder never forgets it.
This isn't a problem with null, it's a problem with allowing the user to
make a mistake.
- Sjoerd Maessen at Tue, 03 May 2016 11:22:54 +0200
Good article, I agree with the fact using null can make it harder to debug
in most cases.
A small addition about the NullLogger. I think the code could be improved by
making the Logger instance non-optional in the constructor. If you always
require an instance of Logger you remove the dependency to NullLogger and it
keeps the service class cleaner. I believe it's not the responsibility of
the "SomeService" class to make an instance of NullLogger and fill the
property. But it's the responsibility of the callee to provide an instance
that will be used.
- Andreas Czakaj at Tue, 03 May 2016 13:37:54 +0200
Kore, thanks for your interesting post. Null object is indeed a pattern that
is used too rarely.
In addition, I'd like to hint at yet another common pattern that is
frequently used in functional programming, i.e. Option Type, cf.
https://en.wikipedia.org/wiki/Option_type
PHP seems to have no native support for it (yet) but it should be feasible
to code something similar.
Cheers,
Andreas
- Saji Nediyanchath at Wed, 04 May 2016 04:03:09 +0200
Feels like a pretty good idea and an easily implementable one as well.
Thanks for sharing it.
- Romanko at Wed, 04 May 2016 07:59:07 +0200
Agree with the first example. But totally disagree with the last one as it
is too ambitious.
Consider this:
# All PHP variables by default are NULLs until initialised. Because it is a
default behaviour it is better to remember about it and validate / verify
your data or properties your class has before making calls or whatever.
# A number of built-in PHP functions may return NULL and so it may bubble up
into your code. See above.
# In many cases NULL as a return value literally means "no data" and so
what's a point in throwing exceptions?
# Speaking of exceptions: method1 calls method2 to get data. Method2 does
not have data and it throws an exception. However, for the method1 it may be
perfectly valid not to receive any data at this stage. However, its
intention to get data and continue is badly interrupted with exception
thrown in method2.
Unchecked exceptions must be used wisely. It is all good and fun when you
can clearly see a stack trace and in perfectly designed application it is a
"must" however, your app must do what it is intended to do, you don't
develop with single intention in mind to have a clear stack trace.
- Michael Butler at Wed, 04 May 2016 13:41:23 +0200
A few are saying you should make the logger required in the constructor...
Sure we could do that, but then *everywhere* that calls it would need to be
updated. That may be difficult or impossible if it's not your code.
By defaulting to a nullLogger we are doing it safe and with a far smaller
blast radius of change.
- Henning Kvinnesland at Tue, 10 May 2016 10:41:08 +0200
Masking fatal errors like seems like a terrible idea to me. Instead of a
clear error-message, the new developer has a fully working application which
for "some reason", does not cache or does not log in production. You have
just added another layer of confusion. If the application only fails in
production, what you are missing is a staging area where you can properly
test the application before deployment.
- Pawelzny at Thu, 23 Feb 2017 19:42:35 +0100
Null reference has special meaning: "no value". This is very useful
information. Interrupting application flow with exceptions which are not
exceptional at all gives no advantage for me.
- Sofi at Thu, 23 Feb 2017 23:08:50 +0100
This entire section is wrongly formulated. Like some have already said (you
might wanna comment voting here, I'd vote few up)masking initialization
issues with default "do nothing implementations" is the worst of all
practices as you end up having code that seems to be running all right but
it is not fulfilling its purpose. Why, pardon my words but who the hell
knows... this is cost. Of course null should not be used to mask or hide
away error cases, but this is just point of bad design and one that needs to
improve his/her skills.
The solution in overall null fame is educating people how to use it properly
and not, faking things and saying null is bad practice.