Large Inner classes and private variables

Question

One thing I've run into a few times is a service class (like a JBoss service) that has gotten overly large due to helper inner classes. I've yet to find a good way to break the class out. These helpers are usually threads. Here's an example:

/** Asset service keeps track of the metadata about assets that live on other
 * systems. Complications include the fact the assets have a lifecycle and their
 * physical representation lives on other systems that have to be polled to find
 * out if the Asset is still there. */
public class AssetService
{
  //...various private variables
  //...various methods

  public AssetService()
  {
    Job pollerJob = jobService.schedule( new AssetPoller() );
    Job lifeCycleJob = jobService.schedule( AssetLifecycleMonitor() );
  }

  class AssetPoller
  {
    public void run()
    { 
      // contact remote systems and update this service's private variables that
      // track the assets.
    }
  }

  class AssetLifecycleMonitor
  {
    public void run()
    {
      // look for assets that have meet criteria for a lifecycle shift
      // and update this service's private variables as relevant.
    }
  }
}

So, what can happen if I have a couple helpers and they're at all complex, is the overall class file can get really large. I like the inner classes in that it makes clear the classes are wholly owned by the service and exist only to help that service. I've tried breaking the classes out and passing the parent service as a reference, which works mostly, but things I don't like are:

I end up exposing package level accessors so the broken out classes can get to the variables, whereas before I didn't expose the setters at all since the inner classes had direct access.

Plus, things get a bit more wordy as I'm constantly calling accessors rather than the underlying variables. A minor nit, granted.

Convenience methods (e.g. checkAssetIsValid() or some such) need package level exposure now so the helper classes can call them, where as before as inner classes they could be private.

Even worse, I need to pass the service implementation class to the helper classes constructors since I don't want to expose these helpers methods in the interface the service implements because that forces them to be public. This can create some unit test/mocking issues.

Worse yet, any synchronization I wanted to do gets leaked out through some external convenience method (e.g. lockDownAssets() during a poller update). Before, the internal classes had access to private Locks.

So, in short, breaking the classes out loses some of the encapsulation I like. But leaving them in can lead to some large java files. I've yet to find a good way to deal with this. C++ had the concept of "friends" which I've rarely missed, but would actually help in this case.

Thoughts?

Answer 1

On bytecode level inner classes are just plain Java classes. Since the Java bytecode verifier does not allow access to private members, it generates synthetic accessor methods for each private field which you use. Also, in order to link the inner class with its enclosing instance, the compiler adds synthetic pointer to the outer 'this'.

Considering this, the inner classes are just a layer of syntax sugar. They are convenient and you have listed some good points, so I'd list some negative aspects which you might want to consider:

• Your inner class has a hidden dependency to the whole parent class, which obfuscates its inbound interface. If you extract it as package-private class you have a chance to improve your design and make it more maintainable. Initially it's more verbose, but often you'd find that:
  • Instead of exposing 10 accessors you actually want to share a single value object. Often you would find that you don't really need a reference to the whole outer class. This also works well with IoC.
  • Instead of providing methods for explicit locking, it's more maintainable to encapsulate the operation with its context in a separate class (or move it to one of the two classes - outer or formerly-inner).
  • Convenience methods belong in package private utility classes. You can use the Java5 static import to make them appear as local.
• Your outer class can bypass any protection levels and access private members of your inner class directly. This is not bad thing per se, but it takes away one of the language means of expressing your design.
• Since your inner class is embedded in exactly one outer class, the only way to reuse it is to subclass the outer class. An alternative would be to pass explicit reference to a package-private interface that the outer class implements. This would allow you to mock the outer and better test the inner class.
• Though recent debuggers are quite good, I have experienced problems with debugging inner classes before (conditional breakpoint scope confusion, not stopping at breakpoints, etc.)
• Private classes bloat your bytecode. See my first paragraph - often there is an API that you could use and reduce the number of synthetic cruft.

P.S. I'm talking about non-trivial inner classes (especially ones that do not implement any interfaces). Three line listener implementations are good.

Answer 2

Don't forget to consider why you're trying to break up your large class. Is it for software engineering purposes? E.g. it's a programming hotspot and you have such a large file it causes complicated merges on your developer team?

Is it just a general desire to avoid large classes? In which case it may be that your time would be better spent improving the code you do have.

Is the code becoming difficult to manage, e.g. debugging and ensuring avoidance of unintended side effects is becoming more difficult.

Rick's comment about using unit tests to ensure continued consistent behavior is very valuable and a good idea. It may be that the current design simply precludes refactoring and you're better off re-implementing the same behavior starting from the interface of the original. Be ready for plenty of regresssion testing!

Answer 3

The line between encapsulation and separation can be difficult to walk. However, I think the main issue here is that you need some kind of solid interaction model to use as a basis of separating your classes.

I think it's reasonable to have external helper utility classes which are used in many places, as long as they don't side effect I don't see an issue. It's also reasonable to have static helper classes, as long as they are well organized, which contain often used methods such as checkAssetIsValid(). This is assuming that checkAssetIsValid does not need to access any external state other than the object you are passing it.

The most important thing for separation is not to have objects which share permanent references in many of these classes. I like to look to functional programming for guidance. Each class should not be reaching into the guts of other classes and changing state. Instead each class which does work should produce and consume container objects.

Visualization can be very helpful too. I noticed a thread on the topic of Java Visualization tools here. Ideally, your class interaction diagram should look more like a tree than a graph.

Also, I just want to note that refactoring a large class into smaller classes can be extremely difficult. It's best to build a suite of unit tests for the public interface at the very least so that it will become immediately obvious if you break something. I know tests have saved me countless hours in the past.

Hopefully some of this will be helpful. I'm kind of just rambling on here.

Answer 4

I'm not a fan of excessive use of inner classes. I think that they don't really offer any advantage (when used in the extreme) that putting the code in a normal class wouldn't, and they just serve to make the class file unnecessarily large and hard to follow.

What's the harm if you have to increase the visibility of a few methods? Is it going to completely break your abstraction or interface? I think too often programmers tend to make everything private by default, where there isn't really much of a harm in letting some other classes call your methods - if your design is truly OO-based, that is.

If all of your "inner helper classes" need access to some of the same methods, consider putting them in a base class so they can be shared via inheritance.

Answer 5

Yeap. Probably you need to re-refactor those helpers and not move them all as they are. Some things belong to the service some other to the helper. Probable new classes should be used to encapsulate the data.

One posibility you can use is to AOP to provide fine-grained access, and include in the point cut that the method should be only invoked from the "friend" class. Still your method would be exposed :(

I guess there is no easy solution for this.