Monday, 12 December 2011

Some of My Coding Habits

This is a brief description of my coding habits including indentation, new lines and other things that I don't usually see although I think it makes code clearer.

Variable declaration and assignment on separate lines

JLabel nameLabel;

nameLabel = new JLabel("Name:");

Although I didn't take up SmallTalk seriously I did come away with, among other things a new coding habit.  SmallTalk variables are always declared in pipes i.e. | myVar | before use.  In Java I use this, especially when the object needs one or more method calls after it is created.  It is almost like a paragraph header and allows the code on the left hand side to match up which I find quite easy to read.

Static imports

No need for an example here, I just really like static imports... in moderation.  They are particularly effective for enums where the enum name might be repeated over and over again.  It is also quite safe to use enums in this way because type safety will not allow you to do anything silly such as EnumA.RED == EnumB.RED without giving a compilation error.

Static imports are quite bad for importing primitive constants, it is much easier to make a mistake without the context of the class name from where it came.  For example, two different integers might mean very different things.  A number of cats and a number of dogs which should not be added together. 

It is quite rare for me to write static methods so this isn't usually an issue.

Returning new

public String findDayName(int dayIndex)
  assert dayIndex >= 1 && dayIndex <= 7;

    case 1: 
      return "monday";
    case 2: 
      return "tuesday";
      return null;

  // and when using an if statement
  if(dayIndex == 1)
    return "monday";
  else if(dayIndex == 2)
    return "tuesday";
    return null;

When using a reasonable size switch or if statement I typically bring this out into a separate method with no variables other than the data needed to perform the switch.  Even this is not necessary if the data is a class field. There is no variable that gets assigned to so no need to worry about a name, or maintain the name and the method name make sense together.  If statements also do not have brackets to cut down on needless lines and white space which in this case would just make the code more bulky and difficult to read.

The if statement is reminiscent of Erlang guarded methods.

Saturday, 15 October 2011

Resolving Bugs

There are at least three roles in a bug report.  The first two are the bug reporter or owner of the bug who is equivalent to the Customer or Product Owner and the Software Engineer responsible for the bug.  An Architect, Technical Lead or Project Manager may exist who has more direct access to the customer, this is called QA in Bugzilla.  In certain cases the Engineer may need to escalate issues if they cannot resolve it themselves.  This can happen all the time, the Customer might raise a bug stating that functionality is missing when it is not in the requirements.  If this is not dealt with in a way that includes all stake holders, it could cause issues immediately or later on in the project at demo time.

Below is a table of resolutions and the circumstances in which they can be applied and by who.  The only possible option for the software engineer is to fix the bug or agree another resolution with the customer, or team lead.  This means that the bug will always be fixed or dealt with in a final way where the specification will be updated to reflect the new behaviour (or new understanding of behaviour) or at least a resolution agreed which should be traceable in the bug comments.  If the bug is not fixed and not agreed then it is bound to come back in the future starting with the line “I reported this months ago and it is still NOT FIXED”!

Only the Software Engineer can set.
Only the Customer can set.
Won’t Fix
Architect, Technical Lead or Project Manager can set based on agreement with customer.  Should be extremely rare.
Never set
Never set
Software Engineer can set in the interest of efficiency.  Be careful that this is truly a duplicate and the customer is in fact not describing some other behaviour.
Works For Me
Never.  Negotiate with the Customer for them to set the bug to Invalid instead.

Sunday, 25 September 2011

Detangling Dependencies Between Guice Injected Services

It is very easy to allow services to depend on each other, even when they are in different modules.  In fact Guice explicitly supports this behaviour with the AbstractModule#requiredBindings method.  I think this is very bad and in my experience has led to some complex interactions and strict requirements on order of creation.  Even worse, in some cases you may require a cyclic relationship to fulfill what you want to do which means you can't fully initialise the services via constructors.

Instead of a clever technical solution, I believe it is the software design that is incorrect.  The issue can be resolved using SOLID principles to identify and rectify the issue.

The services require the dependencies because they are trying to do too much, this is violating the Single Responsibility Principle (SRP).  I often think of SRP as a DON'T whereas the Interface Segregation Principle (ISP) is a DO: to fix an SRP issue apply ISP and move that behaviour out into new classes.

This still hasn't fixed the issue yet because those dependencies still have to be wired up in the modules, except you don't.  The new classes with dependencies can be instantiated via Guice when required with injected dependencies, the code to wire them up in the modules can be removed altogether.

In this way, layers of objects are being created.  The bottom layer of services are injected into the next layer of composite services which are then injected into the application.  This is the Open Closed Principle (OCP) where classes should be open for extension but closed for modification.  In other words, prefer to building on top of existing classes instead of modifying them.

I do suggest that all layers can talk to each other unless the software is huge enough to justify such abstraction.  Also, be sceptical of composite services which simply provide a layer over another service and perhaps wrap exceptions or even provide a something useful.  In this case a new API has been created to replace a more universal one.  Ask if the new API is actually better than what it is truly wrapping and it is worth the new layer being imposed.

Guice Custom Scopes

Guice custom scopes are, for some reason badly explained in almost all web documentation that I was able to find.  All implementations cover extra Guice concepts or muddy the code with concurrency handling while not describing the main concept which is surprisingly simple once realised.

A scope is a condition for reusing injected objects, instead of creating a new one each time.  For example, if a network connection was being injected then the scope would be while the network connection was open, reuse the object.  When it is closed, open and inject a new one.

The implementation for this is to basically provide a factory object,  implementing the Scope interface.  This has one method:

Provider scope(Key<T> key, Provider<T> unscoped)

The Key object defines the injected type, the unscoped Provider can be used to create a new instance.  Typically the returned Provider implementation will  create the required object using the unscoped Provider and keep a reference to it while in scope.  When the object is requested again the stored object is returned if still in scope.  This example usage above is how I needed to use custom scopes but it is flexible enough to do all kinds of weird things.

Here is the Scope implementation.  It is a bit more complex than necessary because for this example I wanted to allow objects to be created out of scope.

Here is out annotation which we will use on classes that can be stored within the scope.
The interface to inject:
The class that implements that interface to be injected.  Note that we have annotated this with our @BookScoped annotation.  The constructor will display new objects being created.
Now our Guice module.  The scope annotation is bound to an EnterableScope instance which we'll keep a hold of.
Finally the main class to demo the scope.
When the main class is run it outputs the following to the command line:

entering scope
created customscopes.Hobbit@12a54f9
exiting scope
created customscopes.Hobbit@30e280
created customscopes.Hobbit@16672d6
created customscopes.Hobbit@fd54d6
created customscopes.Hobbit@1ccb029

When within scope the four calls to #getInstance only create one instance, when not in scope a new instance is created each time.  That's a whole lot of code although it is all fairly simple.  From a maintainability viewpoint I do see some issues with this, scopes are so arcane to anyone not experienced with Guice that it would be quite easy to misuse a scope which could cause all kinds of issues with retained state and possibly memory leaks.  All I can suggest is document everything and get this over to the team.

Tuesday, 30 August 2011

Checked Exceptions Part 3

I found a bug last week that was caused by a checked exception anti-pattern or maybe code smell. I name this smell "foo() throws Exception". The premise is that a method throws Exception, Throwable or any other exception which is low enough down in the class tree to capture the exceptions which may be thrown by the code that is being worked on. The software engineer happily beavers away without realising that the code is throwing all kinds of exceptions that are not being handled correctly. The handler is higher up the method chain and detached from what the code is actually doing so cannot reliably act, most likely it is simply logging exceptions. Once handled and logged nothing more is done possibly leaving the thread and/or application in an invalid state.

This can be caused by the programmer wanting the exception handling to go away perhaps because exceptions were used too liberally and now there is more exception handling than business logic. Maybe handling the exception is too uncomfortable since there is no plan B, there is only failure, but the application is long running and cannot be restarted so a catch-all was a last resort for an error that supposedly could not happen.

The application in question simulates a GSM-R application and calls are made to get the active call which the application is currently dealing with.  The other party could end the call at any time and the system is completely asynchronous and multi-threaded and it is highly likely that no call could exist the next time you check for it.  The exception being thrown was part of the null-checking strategy which has proved highly effective in reducing NPEs.  Instead of a null being returned from a method, a checked exception is returned: CallDoesNotExistException.  So this exception could happen in normal application flow and must be handled by application logic.

As it turned out the fix was remarkably elegant once the functionality as a whole was understood (read remembered).  A catch for CallDoesNotExistException was added to the catch-all handler which would navigate the user out of the process they were in to one of the two possible application states fixing similar issues in the entire application.  This change was helped by the fact that CallDoesNotExistException is task specific and other parts of the application could easily identify the issue in the code, as opposed to a generic exception which would be almost impossible to identify whether it was checked or not.

Wednesday, 3 August 2011

Using Method Handles

A few weeks ago I ran some tests pitting method handles against typical reflection to see what was faster. The result was that reflection was faster although since it was a quick test I don't have the result data or what run configuration I used. Now that I'm tasked with looking into porting our core systems to Java 7 I thought I'd take a more critical look.

Below is the source code I used to run the test. It calls a method on a class called Incrementer a million times. I didn't notice any difference when running the methods several times when running the test several times before timing values to "warm up" server mode.

Note that this code throws an exception when run from eclipse. I had to use the javac/java executables from the command line for this to work. When running the class in mixed mode I got the following results:

java -verbose:gc MethodHandleTest
[GC 4416K->141K(15872K), 0.0011714 secs]
[GC 4557K->141K(15872K), 0.0006216 secs]
[GC 4557K->141K(15872K), 0.0006387 secs]
reflection took 515ms, result is 1000000
method handles took 16ms, result is 1000000

There is plenty of garbage collection happening but not enough to account for the huge time difference. When running in server mode I got the following results:

java -verbose:gc -server MethodHandleTest
reflection took 47ms, result is 1000000
method handles took 15ms, result is 1000000

java -verbose:gc -server MethodHandleTest
reflection took 32ms, result is 1000000
method handles took 31ms, result is 1000000

All runs of the test gave almost identical results to the above where method handles would be twice as fast as reflection or equal with no garbage collection. Interestingly, if I comment out the increment of the long value the same result as with the increment is produced:

java -verbose:gc MethodHandleTest
[GC 4416K->141K(15872K), 0.0011488 secs]
[GC 4557K->141K(15872K), 0.0006595 secs]
[GC 4557K->141K(15872K), 0.0006482 secs]
reflection took 500ms, result is 0
method handles took 31ms, result is 0

This makes me think that incrementing the long is a very small proportion of the time it takes for the reflection example to run and the majority is for calling the method through reflection. In server mode the reflection call is JITted and almost identical results are produced. This is mostly conjecture since it is quite difficult to work out what the Hotspot compiler is doing.

In conclusion, method handles consistently performs as good as, or better than reflection no matter what run configuration you are using. Stability is an issue with the current tools, eclipse will have to support Java 7 before I could use method handles in our systems.

Saturday, 7 May 2011

An Immutable List in the JDK

It's quite strange that there is no immutable list class in the JDK, immutability is most likely the biggest part of any concurrent design.  It's well understood that an immutable object can be safely passed or accessed by other threads meaning that it is not necessary to lock.

It's not quite true that there is no immutable lists in the JDK, there is always the Collections#unmodifiableList method, return a wrapper around the supplied List that does not allow modification.  However, it is only a wrapper and the underlying data structure can be modified so there are no guarantees about immutability, only conventions which are only convention if you knew about it in the first place.  Or the guy before you did.

The problem with the unmodifiable list is that once you have it, there is no way to check if it is unmodifiable.  The implementation class, UnmodifiableList is package protected so it is not possible even to do an instanceof check.  I have found this to be a big source of inefficiency.  There is no way to check if the list is immutable so if you must have an immutable list you can "ask" via documentation that lists should be immutable or defensively copy the list.  Typically I would using defensive copying, because there is no way to know how the code is going to be used in the future and if the engineer is going to check the documentation.  I'm not even sure that it is a reasonable requirement to ask someone to check the small print for every method they use.

The fix for this is relatively easy and even doable without hacking the OpenJDK by using the -XbootClasspath command line option.  Using this you can specify the boot classes you want to load, by default its your JRE rt.jar.  If you modify some JDK classes you can specify that to be loaded first and then rt.jar so your modified classes will be first to load.

All that needs to be done as a starting point is to make the UnmodifiableList class public and make that the return type from Collections#unmodifiableList instead of List so we know what we're getting and other classes can pass it around also.  It doesn't fix the possibility of the data structure being modified but we're 80% of the way there for such a small change.  Because UnmodifiableList is still a List implementation all current code will still compile. 

Unfortunately it is not legal to deploy this change to a production system, it's only for fun.