- GLOBAL STATE IN SHEEP'S CLOTHING -
There are not many things that can get me in a ranting mode, but statics are high on the list. In Java statics are the main source of hard to reproduce bugs, untestable code, and bad architecture. In my opinion, every time the keyword static is used, a red flag should be raised. There are some use cases where statics make sense, but they are very limited. The bad use cases however are abundant. They start out as nice little helper thingies, just some little piece of reusable code, not worth an instance, and before you know it, they infect your codebase and make it one uncontrollable mess of... code.
As article 19 of the classic story about How to Write Unmaintainable Code states:
Never use local variables. Whenever you feel the temptation to use one, make it into an instance or static variable instead to unselfishly share it with all the other methods of the class ... programmers can go a step further by making all variables global.
Basically everything which uses statics are forms of global variables, and global variables shouldn't be treated lightly. If your architecture relies on some magic global state to be there, rethink your architecture. Like I said in my post about JMockit, new is also a form of a static method, I'll also cover that.
The Good
Like I said earlier, I am not calling for a ban on the static keyword. There are some examples where it is fully acceptable and good practise to use statics. An undisputed use case is constants, ie:
public static final int SOME_CONSTANT = 100;
Any kind of constant should be a static final, even better would be a proper constant keyword, but since it doesn't exist in Java, we should do it like above. Enums are just a variant of constants, so feel free to use them.
Next on the list we go already a bit more shady, but I'll allow it, static factory methods.
public static SomeClass create(SomeDependency dependency) {
return new SomeClass(dependency);
}
In its purest form it doesn't bring much, the above example is just a wrapper for new. More complex examples are for example Intent creations in Android. Any parameter you would want to give to an Activity via an Intent will need to be broken down into parcerable things.
public class DetailActivity extends AppCompatActivity {
private static final String PRODUCT_KEY = "PRODUCT_KEY";
public static Intent createIntent(Context context, Product product) {
Intent intent = new Intent(context, DetailActivity.class);
intent.putExtra(PRODUCT_KEY, new Gson().toJson(product));
return intent;
}
}
Here we see all the knowledge that is needed to create an Intent to let Android start a new Activity nicely packed in that Activity class. Activities cannot be newed themselves, we need Android to do that for various reasons.
The last other valid use case is static inner classes. Sometimes you need an inner class, which is only used in one location, and you do not want it to capture the outer class.
The Bad
Alright, we covered all proper use-cases of statics. Any other use-case of statics are just global state, which should be something we do not want. Before we go into the ugly things, let's look at the bad things. These things are just an early stage of the ugly things. When not addressed, within a couple of sprints this will grow like a tumor and will definitely end up as the ugly things.
public class DateUtil {
public static boolean isMonday(Date date) {
return date.getDay() == 1;
}
}
The above code looks nice, it's focussed and to the point. However, it has created a precedence. We just added our first static method that is not creation. It executes logic, and returns a conclusion. If we can do this once, let's do this again.
public static boolean isSameWeekNumberForLocalesWhereMondayIsNotTheFirstDayOfTheWeek(
final int weekNumber, final DateTime dateTime) {
if (isPreviousWeek(weekNumber, dateTime.getWeekOfWeekyear())) {
int dayOfWeek = getDayOfWeek(dateTime);
int firstDayOfWeek = getFirstDayOfWeek();
if (firstDayOfWeek == dayOfWeek) {
return true;
}
}
return false;
}
This is already way more complex, we have a method that calculates if the weeknumber is as expected for locales that not have monday as the first day of the week. In the US for example, officially the week starts on Sunday, in Egypt it starts on Saturday, but in Europe it starts on Monday. This static method utilizes several other private static methods, and even though everything is stateless, it feels we are on the wrong path. We started with just a nice simple method that returns if today is Monday, and before you know it, we have complex logic in global state. But wait, it get's worse...
Every kitten grows up to be a cat. They seem so harmless at first--small, quiet, lapping up their saucer of milk. But once their claws get long enough, they draw blood, sometimes from the hand that feeds them.
The Ugly
This is where the fun starts. This is the place you go to laught together with your developer friends, mining your next section of The Daily WTF. These examples are so bad, you will never ever create something like this. You will never even encounter something like this, since y'all have proper architecture. Until, someday, you spend 2 man weeks on a seemingly easy bug, that you eventually lead back to a static field. Statics are the root cause of many difficult bugs, since global state is so hard to manage.
PaymentEndPoint.setInstanceForTesting(new TestEndPoint()); // Method for Testing
CurrencyConverter.setTargetCurrency(Currency.EURO); // Static state
CreditCardService.init(500); // Static initializer
Payment payment = new Payment(100, Currency.US_DOLLAR);
payment.execute();
...
public void execute() { // Obfuscated dependencies
CurrencyConverter converter = CurrencyConverter.getInstance(); // Singleton
int convertedAmount = converter.convert(amount, currency);
CreditCardService.bill(convertedAmount); // Static business code
}
There are so many things wrong with the above example, I hardly know where to start. We can choose from: methods for testing, static state, static init methods, singletons, static business code methods and obfuscated dependencies. It's the ultimate nightmare. Have you ever tried to explain something like this to someone.. did you ever write tests for something like this?
Singletons
Let's start with my number one anti-pattern, the classic singleton pattern. If anyone ever says that singletons are good practise and should be more commonly used... I'm not gonna finish that sentence. Singletons are the prime example of global state. Just out of the blue any class can get the instance of this object, and call methods on it. Why not just make all methods public static methods? You cannot ever trust the state of this class, since everywhere in your code it can be influenced. It defies the rules of OO programing.
Static init methods make singletons even worse. As is just global state isn't enough, let's initialize it first, and from then onwards we can just trust the state to be there. So you then have 2 methods on the class itself, but wait there is more.
For testing purposes you might need to inject a different state, since you need to be able to control it. How else are you going to insert your mock? You'll need a setInstanceForTesting(...), because otherwise the global state will be wrong. Just set the state only in testing, never in production, because no one will ever use a *ForTesting(...) method in real life. But if you ever do, just add a // TODO: This should be fixed to that call, so you know it should be fixed.
Finally leakage of global state is an issue. So let's add a destroy() method, so we can always clean up after ourselves. It wouldn't be nice to leak global resources, now would it?
public class PaymentEndPoint implements EndPoint {
private static EndPoint instance;
private SomeDependency someDependency;
private PaymentEndPoint(SomeDependency someDependency) {
this.someDependency = someDependency;
}
public static void init(SomeDependency someDependency) {
instance = new PaymentEndPoint(someDependency);
}
public static EndPoint getInstance() {
return instance;
}
public static void setInstanceForTesting(EndPoint testEndpoint) {
instance = testEndpoint;
}
public static void destroy() {
instance = null;
}
...
}
Doesn't it seem funny, that you now have an init method, something to get the instance, a hack to swap the instance, and a hook to destroy the instance? Why not just use a constructor, an actual instance, and dereference it when you are done? What did the singleton actually add? If it is really important that there is only one instance, create it in your main entry point and pass it around. Have tests verify the correct working of your app, do not use global space for this.
public class PaymentEndPoint {
public PaymentEndPoint(SomeDependency someDependency) {
this.someDependency = someDependency;
}
...
}
Obfuscated dependencies
When you decide to use global space to get a hold of dependencies, you are not reducing the number of dependencies of a class. The mere fact that it isn't visible in the constructor, or on any method signature doesn't mean the dependency is gone. You didn't decrease complexity, you increased it! When a class explicitly asks for a dependency there is no way for you to miss it. But if a class uses global space to get a dependency, how would you know this from the class's signature? You can't, thus increasing the complexity, while maintaining the number of dependencies.
Below code is an abstract of the example given earlier;
Payment payment = new Payment(100, Currency.US_DOLLAR);
payment.execute();
From the signature, how can you conclude that this depends on CurrencyConverter, CreditCardService or PaymentEndPoint? The dependencies are obfuscated, not gone, just hidden, see below.
public void execute() {
CurrencyConverter converter = CurrencyConverter.getInstance();
int convertedAmount = converter.convert(amount, currency);
CreditCardService.bill(convertedAmount);
}
How about changing this static stuff which might depend on far more global space dependencies, and rework to something like this?
public void execute(CurrencyConverter converter, CreditCardService creditCardService) {
int convertedAmount = converter.convert(amount, currency);
creditCardService.bill(convertedAmount);
}
You can see the dependencies of this method, you have a seam to do your testing on, no globals needed.
New times
new is also static, why? By newing something you just create it out of the blue - out of global space. No seams for testing. However, we need some way of creating objects, how to solve this? Well, by making classes focussed, by respecting the single responsibility paradigm. Either you class is responsible for execution, or it is responsible for creation. That's right! Creating objects is a full fledged responsibility, do not treat it a second class citizen. Dependency Injection can get you only so far, at some point you have to new something, why not create a single class that is responsible for wiring up dependencies? Like I said earlier, static factory methods are a bit shady, but why not have a non-static factory? You could have a factory object instance who's sole responsibility is to wrap new, to construct. You will need to pass this factory to any class that needs to construct something, which makes it clear on the signature of that class that it depends on object creation. The dependency was always already there, this new object just makes it visible.
Do not overdo this factory thing, data classes - for example ArrayList - should just be newed where they are used.
There are plenty frameworks that do this boiler plate factory things for you, which could be an entire new post, so I won't dive into this. You can take a look at Dependency Injection Frameworks like Dagger.
Conclusion
If you need to pass around certain classes all over your application, don't make them singletons, rethink your architecture. Global space greatly increases the complexity of your system, while pretending to reduce it. It reduces testability, and increases coupling. If you would develop in a test driven manner, you will almost never create statics. You will need to deal with statics, since often system resources are static, think about things as logging and hardware resources. This doesn't mean that you should follow suit. Especially with third party libraries, try to encapsulate static references, and deal with instances. Try not to overdo it, concepts require practise. There are always exceptions to rules, if you come up with a proper exception, then use statics. However, if you do not have a good reason why something should be static, stop, and refactor to an instance. Statics should be a last resort, not a go-to-pattern.
On Android, static instances tie to the lifetime of the VM, not to the lifetime of the App. This means that you are never certain your static references will be fresh. When the OS decides to kill your app for resources, but keep the VM alive for fast restart, your singletons will remain, eating away resources in the meantime. State will survive the lifetime of your App, think if this is something you want.
References
If you want to read more, have other views, and find more interesting content:
How to Write Unmaintainable Code
This site is considered a classic with programmers, it jokingly described how to write really bad code. The link provided is not the original site, but according to Google, malware has been find on the original location.
Mockito vs JMockit, battle of the mocking frameworks For years the industry standard of mocking on the JVM has been Mockito. This post shows why JMockit should be the new standard, since it brings many many advantages over Mockito, with little to no drawbacks.
Dagger
Dagger is a Dependency Injection framework, which makes it easy to construct complex classes, and make plentiful seams for testing. Dependency Injection is made easy, and once you get the hang of it, you will not want to go back.
The Clean Code Talks -- Unit Testing
Years ago I came across this talk, it nicely explains the problem with obfuscated dependencies and how to circumvent this. How can we write proper, isolated tests. It's at a nice entry level, so I really recommend it.
