Static methods are a clue that a piece of code might not be conforming well to object oriented design principles. Its probably not as well designed as it could be, or as reusable as it could be. Most of all, its probably not as testable as it could be. The contents of the method often belong on one of the objects being worked on, and sometimes belong in a whole new object unto themselves.
The first clue for how to fix it is to look at the parameters being worked on in the method. If it is, then you can convert the method to an instance method on that object. IntelliJ IDEA has a refactoring option called "Convert To Instance Method...", which will ask you which parameter to move the method to. If you're not using IntelliJ IDEA, you can move the method to the appropriate Object, delete the parameter, and use this instead.
public class AccountFunctions {
public static void withdraw(Long amountInCents, Account account) {
// ...
}
}
becomes:
public class Account {
public void withdraw(Long amountInCents) {
// ...
}
}
The second clue for how to fix it is to look at the local variables being used in the method. It's possible that the contents of the method belongs on one of them instead of one of the parameters. Maybe a singleton object is being accessed in the method for example. In this case, the solution is similar to the first option, except that the parameters all still exist.
public class CartFunctions {
public static void addItemToCart(Long itemId) {
Cart cart = Cart.getInstance();
List items = cart.getItems();
items.add(itemId);
}
}
becomes:
public class Cart {
public void addItem(Long itemId) {
// ...
}
}
The third clue for how to fix it is to create a completely new class just for the static method. The parameters on the method become instance variables, and a constructor should be created that takes them as parameters. The method itself becomes an instance method on that class that uses the instance variables instead of having parameters to it.
public class DatabaseFunctions {
public static String buildSelectStatement(String tableName, Collection columns,
Collection conditions, Collection orderBys) {
// ...
}
}
becomes:
public class Query {
private String _tableName;
private Collection _columns;
private Collection _conditions;
private Collection _orderBys;
public Query(String tableName, Collection columns, Collection conditions, Collection orderBys) {
}
public String toSql() {
// ...
}
}
Comments
When an algorithm is operating on an object, rather than being
an operation of the object, then it makes sense to implement it
outside the object. That is, the static method makes sense, or if you
need more flexibility (perhaps there is more than one way to withdraw
money from an Account) then you can make a separate object, an
AccountWithdrawStrategy, or in English, just call the interface
Withdraw, or Withdrawer.
Static methods don't conflict with OO, they facilitate it. OO doesn't
work alone. In fact, when you don't need substitutability, static
methods are often the best solution.
An argument in favour of non-static methods in Java is that
they are convenient. account.withdraw(100) as opposed to
AccountUtility.withdraw(account,100). This is just syntax, with the added benefit that you don't need to know where 'withdraw' is defined, at every single use site, in the former. Scala has
'views', that let you view an object as having not only its own
methods, but the methods of a separate object/class. This is good,
because it means that the Account class/interface itself stays easy to
maintain (and hence easy to subclass/implement), and because it
doesn't modify the object itself; unlike in Ruby, the view (mixin) is
purely compile-time.
You can even have two different parts of code having two different
views of the same object; i.e., one bit of code doesn't enforce its
view of the world on others. Speaking of which, if you're going to
say something like:
"Static methods are a clue that a piece of code might not be
conforming well to object oriented design principles. Its probably not
as well designed as it could be, or as reusable as it could be. Most
of all, its probably not as testable as it could be."
You should back it up with reasoning. Static methods are the most
convenient to test (assuming they don't mutate anything).
Posted by: Ricky Clarkson | January 8, 2007 08:40 AM
"Static methods don't conflict with OO, they facilitate it"
Quite the opposite, static methods remove polymorphism that is central to OO and testability. IMO one of the chief losses of static methods is the inability to mock them.
Posted by: Kristopher Schmidt | January 8, 2007 11:51 AM
Firstly, the method itself is easy to test. Once that is tested, it is arguable as to whether you need to mock it to test something that depends on it. Secondly, wrapping a static method to make it substitutable is trivial. You shouldn't penalise the users of a method solely to make testing easier. In fact, imagine you were using a language such as Lisp, where you could redefine a method - you could do that in your test, without changing the code that uses the method. If you recognise that 'objectifying' the method is a workaround for a language limitation, that's a step forward.
Making it substitutable adds complexity. E.g., code that uses Math.min does not typically need to mock Math.min, it just uses it, because Math.min is tested elsewhere.
I discussed this on my blog a few months back.
Posted by: Ricky Clarkson | January 8, 2007 12:14 PM
Ricky - code smells are typically a indicator that something *might* be wrong. That doesn't mean that it is though. You'll see language like "clue", "might" and "probably" in my post. No need to rush to the defense of static methods just yet, I haven't shot them down. You need to use your own olfactory senses to tell you whether a bit of code needs to be improved or not. I'm just giving you some help if you actually decide to move forward.
Kristopher is spot on with his comment about mocking. Once you call a static method, you've eliminated interaction based testing as an option for that bit of code. That might be ok for an integration test, but pretty much sucks for a unit test, especially if that static method is an expensive operation.
Posted by: Marty Andrews | January 8, 2007 04:47 PM
"The first clue for how to fix it" strongly suggests that you think it's broken in the first place, despite the earlier hand-waving.
The fact is that calling a static method is the simplest thing to do. Perhaps it would be better to look for ways of replacing a static method for test purposes, instead of making the code more complex simply because you don't know how to replace a static method.
A static method itself isn't a bad thing - if you want to call it indirectly you can with a trivial wrapper. Java 7's function types and method references should make this even more trivial.
Most static methods that are themselves testable aren't actually expensive operations. I'm not suggesting to use untested external code in a test, but to keep things simple.
I don't know why a unit test cannot use code that is tested in another unit test. The wikipedia article says "Unit testing helps to eliminate uncertainty in the units themselves and can be used in a bottom-up testing style approach. By testing the parts of a program first and then testing the sum of its parts, integration testing becomes much easier." This suggests that unit tests can be built up until they are usable as integration tests - i.e., making the unit bigger and bigger.
The mocking approach seems difficult to scale, and harms the simplicity of the code, in the implementations that I've seen.
By difficult to scale I mean, given some bad code with lots of coupling, it's hard to write a test for, because there are too many objects to mock. Then it's a bit fragile; you don't want to make the code better because then the test will need changing. I'd rather write test code that tests the aims of a unit, rather than the details of its implementation. That way I'm free to change the implementation without rewriting the test.
Posted by: Ricky Clarkson | January 11, 2007 09:52 PM
"The fact is that calling a static method is the simplest thing to do."
No - its the *easiest* thing to do, and usually means you haven't worked out how to work through the complexity to get to the simple code.
"Most static methods that are themselves testable aren't actually expensive operations."
...but if they are expensive, you're stuck. Why would you choose "most" when you can have "alll"?
"I don't know why a unit test cannot use code that is tested in another unit test."
It can, but if its static, then you've eliminated an option. I don't know why you'd eliminate the option.
"The mocking approach ..."
Sounds like you haven't done much mocking in anger? Using mocking decreases coupling, because it encourages the use of interfaces. That gives you more testable code.
On code complexity - I prefer to have a complex arrangement of simple classes rather than a simple arrangement of complex classes. YMMV. The information here is promoting the creation of those simple classes. How you arrange them is up to you.
Posted by: Marty Andrews | January 12, 2007 01:24 PM
"No - its the *easiest* thing to do, and usually means you haven't worked out how to work through the complexity to get to the simple code."
I don't like 'usually'. 'Usually' usually means that not enough thought has been applied to something (like this sentence), or that one is afraid to make generalisations, for fear of being wrong. I'd rather come up with a rule, then work out when it doesn't work. That's why I don't mind saying things as absolutes. When I get contradicted, often by myself, I'll rethink.
Is there some rule for when static isn't the simplest? Perhaps the rule is that it isn't the simplest thing to do, when the operation is expensive, and it is the simplest otherwise. Another possibility is when it has side-effects - e.g., it modifies objects other than those passed to it, then calling the static method isn't the simplest option.
For (a tired) example, Math.abs is a static method, it's trivial to use, and as far as I know, bug-free. It never needs mocking.
If you would disagree, how would a call to Math.abs look? I have a hardwired IoC 'framework' (framework is a bit too grande for it), so I could call context.getMath().abs(). However, then I'm adding getMath to a context that's passed around everywhere - when there's only ever one version of abs this is overkill.
I could implement my context in a String-based way - ((MathInterface)context.get("Math")).abs(), or using generics and a HashMap -
context.get(MathInterface.class).abs(), but all I've done is take away the compile-time guarantee that context will know about MathInterface. It doesn't stop context from needing to know about MathInterface.
In short, calling a static method is high coupling, but, and here's the magic word again, usually it's localised high coupling, rather than medium coupling spread throughout the app plus high coupling in one area (the bit where you tell context to actually use Math.abs).
"...but if they are expensive, you're stuck. Why would you choose "most" when you can have "alll"?"
In that case, I would make a wrapper so that I can easily replace the implementation.
"It can, but if its static, then you've eliminated an option. I don't know why you'd eliminate the option."
That's the thing about options. If I don't need non-static, then I'll choose static. If I do need it, I won't choose static. Nothing is eliminated.
"Sounds like you haven't done much mocking in anger? Using mocking decreases coupling"
I have actually, and I found myself spending longer making mocks than writing 'real' test code. At one time I made each test case completely independent of each other test case - i.e., each class was tested absolutely on its own. Quality in my code has actually gone up since I stopped doing that, because my tests are more realistic.
One place in particular where it caused some trouble - my code, in normal operation, interacts with a server that I don't control. I made a mocked version of that for testing against, but it didn't quite reflect what the server does. I've deleted that now, and I have a category of tests that interact with that server (hence I can only run them when I'm online). A smarter way would be to grab the code for the server and run it locally, but I'm online just enough for that not to be worthwhile.
"I prefer to have a complex arrangement of simple classes"
Me too, given just those two choices. I really prefer to have a simple arrangement of simple classes. Most instantiable classes in my code only have one method, except where they're modelling the domain. The domain at the moment is an IP network simulator - I have a Card interface, which has basic operations like getIPAddress(), getNetMask(), but functions of those, like getNetBlock(), are implemented as static methods. This keeps the objects small and easy to maintain. I never need to mock getNetBlock().
One more idea - I've never used this, apart from by running code coverage tools, but it seems that some bytecode weaving might make static methods mockable in Java. Perhaps some of the mocking frameworks can already do this.
Posted by: Ricky Clarkson | January 13, 2007 01:52 AM
"I'd rather come up with a rule, then work out when it doesn't work."
Ok - then the rule is to not use static methods. Use instance methods on an object instead. abs() should be an instance method on a Number, not on a Math (what is a Math anyway?). If thats not obvious to you, and you're such a big fan of static methods, maybe you shouldn't be using an object oriented language.
Posted by: Marty Andrews | January 14, 2007 01:57 PM
"Ok - then the rule is to not use static methods."
What is the reason for that rule? Can you name a programming system where there is no static method or equivalent?
Two quotes from Alan Kay:
"OOP to me means only messaging, local retention and protection and hiding of state-process, and extreme late-binding of all things. It can be done in Smalltalk and in LISP. There are possibly other systems in which this is possible, but I'm not aware of them." - this is from 2003.
"I invented the term "Object-Oriented", and I can tell you I did not have C++ in mind."
Lisp's object orientation is much more consistent and flexible than Java's, and is built around equivalents of static methods; instances themselves do not have methods, however, methods can be defined such that when they are called, the appropriate one for the types of the parameters is called automatically (multimethods).
Therefore, static methods do not contradict object-orientation. Even without the above, static methods can complement object orientation; the two are not mutually exclusive.
"If thats not obvious to you, and you're such a big fan of static methods, maybe you shouldn't be using an object oriented language."
I'm a fan of keeping things simple. Calling a static method is simple. Indirection for the sake of it is complex. When that complexity adds no gain, i.e., I don't need to dynamically replace the static method, there's no point.
5.abs() looks sweet, Math.abs(5) doesn't, but there are other solutions to that, e.g., Scala lets you make a 'view' of an object where the static methods from a class are mixed in (mixins) with the methods from an object so that the static methods look like they're part of the object. Beautifully, it has no runtime implications. I don't know whether it specifically allows 5.abs() though. Scala's only on the 'interesting' list; the Pascal-like syntax is a bit off-putting.
Posted by: Ricky Clarkson | January 18, 2007 01:08 PM
Nice posting Marty.
Unfortunately, I often see the same sorts of methods placed in common base classes as (non-static) protected "helper" methods (I am as guilty of this as the next programmer). This seems to lead to similar problems in terms of reusability and testability.
For example, continuing your account withdrawal example, if all the classes that need to do account withdrawals happen to have a common parent class (e.g. BaseAction), then some programmers would consider defining a protected method BaseAction.withdraw(Long amountInCents, Account account). Worse still, if there were no common parent class, some would consider defining one just to hold these helper methods.
In some ways I find this even worse, because now only subclasses of BaseAction can do withdrawals!!!!
Of course, this does not mean that I think you should never use protected methods :-)
Cheers,
David
Posted by: David Kemp | January 18, 2007 09:32 PM
You can in this case override the withdraw(...) method in a subclass used for testing, which means that the instance method on the base class is slightly more testable than a static method on a helper class. Both are bad designs though.
Posted by: Marty Andrews | January 19, 2007 11:50 AM
Dave,
I tend to prefer (sorry Ricky, no rule here) composition over inheritance; small classes with public behavior. I blogged about my experience some time ago here: http://www.redhillconsulting.com.au/blogs/simon/archives/000288.html
Posted by: Simon Harris | January 22, 2007 09:42 AM
Simon, me too, but I make it a rule. So far I haven't come across any cases where I want to break that rule, except when using APIs designed for subclassing. Mixins in other languages might prove (to me) a good replacement; I especially like how they look in theory in Scala.
I also try to make small classes with public behaviour - the ideal being a single method; effectively the object is a function.
I posted a comment on that blog, but I don't know whether that's a good idea, as it's from 2005. Might wake up some sleeping dragons.
Posted by: Ricky Clarkson | January 22, 2007 03:11 PM