Showing posts with label Refactoring. Show all posts
Showing posts with label Refactoring. Show all posts

Friday, November 13, 2009

Four Bits Worth of Dev Advice: Half a Bit More

This is the fourth post in my series of four bits worth of dev advice the first three ([1], [2], [3]) covered the sixteen pieces of advice in headline form and the first eight pieces in a bit more detail. This post goes through another four pieces of advice.

1000: Use symmetric interfaces
When designing an interface we have to decide which methods the interface contains. If you know exactly how the interface will be used - e.g. you're the guy developing the code that uses the interface - deciding what's in an what's out of the interface is pretty straight forward. If, on the other hand the interface will be used in unknown ways - e.g. you're developing a library that others will use - the decisions are harder. In the later case symmetry can be an excellent guide: If an interface has method A that has a logical inverse operation B, the interface should probably have B method. For instance, consider this code from the Rotor project:


class CClassFactory :  public IClassFactory
{
CClassFactory() { }

public:
CClassFactory(const COCLASS_REGISTER *pCoClass) : m_cRef(1),m_pCoClass(pCoClass)
{ }

// snip

virtual HRESULT STDMETHODCALLTYPE CreateInstance(IUnknown *pUnkOuter, REFIID riid,
void **ppvObject);

// snip

private:
LONG m_cRef;
const COCLASS_REGISTER *m_pCoClass;
};


This is a typical factory: It can instantiate a certain type of object through the CreateInstance method. Since this is C++ those objects have to be deleted (or at least logically freed, if some sort of pooling is in place) at some stage. But the factory interface does not help us there, so we can only guess how deletion is supposed to be done. This is a case where introducing the symmetric method DestroyInstance might make sense. That makes the code look like this:


class CClassFactory :  public IClassFactory
{
CClassFactory() { }

public:
CClassFactory(const COCLASS_REGISTER *pCoClass) : m_cRef(1),m_pCoClass(pCoClass)
{ }

// snip

virtual HRESULT STDMETHODCALLTYPE CreateInstance(IUnknown *pUnkOuter, REFIID riid,
void **ppvObject);

virtual HRESULT STDMETHODCALLTYPE DestroyInstance(void *ppvObject);

private:
LONG m_cRef;
const COCLASS_REGISTER *m_pCoClass;
};

Adding symmetric operations to interfaces is not a fast rule. That DestroyInstance method might not be necesarry in the case above - I haven't diged deeply into the Rotor code, so I don't know. But if you're in doubt, and including the symmetric operation(s) doesn't seem un-economic, make the interface symmetric.

1001: Use one abstraction level per method
This a classic from Martin Fowlers Refactoring book: In order to write nice readable methods use only one level of abstraction within any given method. If a method contains some code that operates on a higher level of abstraction, and some code that operates on a lower level of abstraction, the lower level code should be extracted to a separate method.
Operating on only one level of abstraction in each method allows the reader to stay on that level of abstraction when reading the method, which is a lot easier than mentally jumping between levels of abstraction. Furthermore the reader knows that he is going down to a lower level of abstraction when going into the implementation of a method, B, which called from another method, A.
To illustrate take a look at this lovely piece of (in-production) Java code:


private void init() {
try {
server = new ServerSocket(Integer.parseInt(PersistentData.getStringValue(PersistentData.UDP_PORT_MLPI,
PersistentData.UDP_PORT_MLPI_DEFAULT_VALUE)));

javavendor = System.getProperty("java.vendor");
System.out.println("Java vendor: " + javavendor);
if (javavendor.startsWith("Sun ")) {
m_DatagramThread = new
DnDatagramThread("localhost",Integer.parseInt(PersistentData.getStringValue(PersistentData.UDP_PORT_WCTRL,
PersistentData.UDP_PORT_WCTRL_DEFAULT_VALUE)));
}
//snip
} catch (java.net.BindException e) {
System.err.println("A previous instance is already running...." + "\nCannot run application: " + e.getMessage() + "\n" + e);
System.exit(1);
} catch (final java.io.IOException e) { // an unexpected exception occurred
System.err.println("Cannot run application: " + e.getMessage() + "\n" + e);
System.exit(1);
}

m_properties = new HashMap();
m_splashScreen = new SplashScreen();
ErrorDialog.instance().setInitialWindow(m_splashScreen);
ErrorDialogCheck.instance().setInitialWindow(m_splashScreen);

progress(5);
setupDebug();
progress(10);
}
This code opens a socket - one level of abstration - then creates and sends a datagram over that socket - another, lower, level of abstraction - and goes on to display a splash screen - a third, higher level of abstraction. Having those three levels of abstraction means that the reader has to think about sockets, protocol specifics and GUI simultaneously to grasp what is going on in the code. That's not separation of concerns. That's spaghetti.

1010: Grow GUTs
Grow some GUTs while developing software. That is grow some Good Unit Tests. Good unit tests express and test the intended functionality of the code under test. It covers not only the different paths through the code, but all the requirements that the code under test is supposed to fulfill.
Most of the unit tests I see revolve around the implementation details of the code under test rather than the actual functionality. Lots of test suites are build by creating one test method per public method in the code under test. Each of these test methods try to cover one method from the code under test. This usually means that most of the tests only check very superficial things, and then one or two of the test methods test an awful lot of things all at once because try to cover the one or two central public method of the code under test. These large test methods become unwieldy, while still not covering the entire functionality of the code under test. Basically the public interface of the code under test is bad unit test template - a BUTT. And who wouldn't rather grow their GUTs than their BUTT?
For a thorough write-up on good unit tests I recommend Kevlin Henneys StickyMinds series ([1], [2]) on the subject.


1011: Prefer use over reuse
Writing re-usable software is something lots of developers want to do. Writing re-usable software is also really hard. In fact, writing usable is hard. And if software isn't even used it certainly won't be reused.
Moreover aiming for reuse in the first iteration over a software design tends to make the design bloated and unclear (i.e. un-economic), because the requirements to the software are unclear, and because there are multiple scenarios in play that the design tries to accommodate. That sort of bloated software will just end up sitting unused on your hard drive. To counter that, focus on one scenario that you're sure of. Prefer to make something that is used in just that one scenario. Then go on to build something that is used in one other scenario. Then go on to build something that is used in a third scenario. Only then, after a number of instances of use, think about reuse.

That's it for now.

Sunday, July 5, 2009

Refactoring vs. Re-architecting

I was recently gave a talk about refactoring. Beforehand I was asked to include something about "refactoring on a small scale" and "refactoring on a large scale". And how the two compare. I sort of just accepted that at first, but while preparing the talk I realised that "refactoring on a large scale" doesn't really make a lot of sense: Refactoring is about doing code changes that enhances readability, but doesn't change the observable behaviour of the software. That works out really well for localized changes such as extracting a method, renaming something, extracting an interface etc. But large scale changes (like splitting a single-process-application into several processes working together or moving from a voluntary scheduling scheme to a preemptive scheme) are very different beasts. Such changes aren't just large scale refactorings, they are re-architectings, and very different rules apply to the two.

Contrasting refactoring and re-architecting
To contrast the two let me list some important characteristics of refactoring:
  • Observable behaviour is not changed
  • The goal is to improve readability
  • Refactoring is pretty safe when you have good unit tests around the refactored code
  • Refactoring should be applied in an incremental fashion as a series of small steps
  • Refactoring can usually be done pretty quickly. I.e. the cost is low.
  • Refactoring helps keep the code base clean, which in turn helps keep the team velocity up (this is the real motivation for refactoring)
By contrast I find the following to be some of the important characteristics of re-architecting:
  • Re-architecting is usually done to improve one or more non-functional aspects, such as performance, scalability, availability, etc.
  • Re-architecting thus changes the observable behaviour
  • Re-architecting can't practically be done through a series of small steps. It must be done in a couple of big steps followed by a long series of small (refactoring) steps
  • The big steps are risky
  • The big steps are expensive/time consuming
  • Unit tests don't offer much help when doing the big steps
  • Re-architecting requires all levels of tests to be successful (integration, system, functional, non-functional, automated, manual)
To conclude I think that re-architecting is risky business that should only be done after thorough analysis of the cost vs. the benefit of doing the re-architecting. It's not "just" refactoring on a larger scale. That makes it sound too easy.