Programming Retrospective

I've recently had to develop and improve a legacy code base. This has reminded me of a several anti-patterns one should consider when coding. This is by no-means an exhaustive list but conveys some of the most frequent ones in my experience.

Final classes without interfaces

There were a lot of instances where classes were declared as final. This may be a good thing from a security perspective but from a testing perspective it meant that mock classes could not be created resulting in unit tests that were harder to write or could not be written.

There are tools to can create mock objects even without an interface. A particular favourite of mine is Mockito. In fact the mocking of final classes can be overcome through the use of the PowerMock API with Mockito.  However a class with an interface is preferable from an OO standpoint as the use of concrete classes results in tighter coupling between participants.

Lack of Defensive Programming

There are numerous examples of methods without any defensive checks. The caller of this method had a try catch block to catch a NullPointerException, NPE. This is a bad code smell because the onus is on the caller of the method to assert the validity of parameters that are passed to the method in question. This is the wrong place. From an encapsulation point of view, it should be the method that validates whether its input parameters are valid or not. This is a form of Design by Contract where the method checks to see if input parameters satisfies some condition and reacts accordingly. I like to think this as the 'Bouncer' Pattern. If you don't look right, you're not getting in :).

A contrived example is shown below:


This short of approach should also be applied to constructors to ensure the object has a valid state and is fully built before use. Furthermore the throwing of exceptions give more context as to what has caused the failure rather than catching a NPE and having to retrospectively determine what has called that NPE to be thrown.

A favourite API of mine is the Validator class in Commons Lang which lists numerous methods for validation in different contexts.

Exposure of super state to child classes

I've seen a lot of instances where child classes would use the state variables of a super class. Variables had protected visibility so there was naked access to these variables which are inherently dangerous. This is because a child class could change the reference to a super class variable with unforeseen consequences.  I think the intention was to provide access to a child class with a parent's state so the child class can perform an operation. A quick fix would be to only allow these parent state variables to be access through accessor methods.

http://c2.com/cgi/wiki?InappropriateIntimacy


But this begs the question on why child classes needed that sort of access in the first place. IMHO, private state should never escape the confines of a class, only behaviour.

The other smell was that the super class was becoming top heavy with functionality. Probably the reason was that common functionality was pushed up the class hierarchy for re-use by child classes. But this resulted in the super class becoming bloated and unfocused. A better solution would be to use delegation techniques rather than inheritance.

http://www.refactoring.com/catalog/replaceInheritanceWithDelegation.html

When I look at a class functionality, I like to keep in mind a Unix philosophy i.e. Do one thing but do it well. If you find your class not adhering to that maxim, that's a sign refactoring is in order.

Printing out error messages to console instead of logging

I've came across a few situations where exception stack traces were dumped to console. Don't do that. Use a logging framework such as log4j. If log4j is used then a console appender can be used to achieve the same goal. Furthermore errors and warnings could be logged to a specific destination i.e. a file so one can see only pertinent errors and not worry about debug messages.

Another observation was the lack of categories used in logging. Most of the statements I saw were a generic dump of error messages for the whole platform. Using categories allows log messages to be sorted. For example I could have a category called com.acme.X for X related logs and com.acme.Y for Y related logs. If I wanted to see all logs I create another appender that logged at the com.acme level.  At the very least, use the fully qualified name of the class the logger resides in as the logging category. The use of categories results in greater capabilities on what should be logged and where it should be logged to. In this example X and Y are logged to different appenders but the possibilities are endless depending on how the categories are devised.

Classes with unclear focus

There were examples of classes trying to do too much. An example would be a Handler class. The main functions are listed below

• Setup relevant properties needed by the handler
• Handle requests.
• Convert a request to a protocol specific message.
• Handle synchronous and asynchronous responses

This resulted in a class of over 1000 lines with several private methods.  This was one of the major smells in that many private methods were only working on a subset of the private fields of the class. This implies that the class is trying to do too much.

Most of the examples I've seen are that the majority of functionality is realized inside one class instead of being delegated to other classes. The lack of delegation means the intention of the class is lost. Furthermore testing of the handler becomes more problematic. By delegation, each of these functions can be tested in isolation.

http://c2.com/cgi/wiki?LongMethodSmell
http://c2.com/cgi/wiki?GodClass

Unwieldy or unneeded comments

There were a lot of instances where code comments were of no use or didn't add extra information. For example, one method contained a lot of retrievals from a database along with ambiguous looping constructs. Each part of the method contained a comment explain what the next section of code would do. The reason I don't like this firstly comments are deodorant on 'smelly' code. That comment is probably there because the code is not clear enough to be understood. Secondly comments are brittle. If I changed that section of code, then I have to remember to change the comments, another piece of maintenance.

I am a proponent of 'Programming by Intention'. This is a programming style where you give meaningful names to methods, variables, classes etc so that the intent of the object in question is clear. Dave Astel gives an excellent overview here: http://www.informit.com/articles/article.aspx?p=357688

In the case of the Handler class, the method was essentially doing three things:

• Obtaining a customer ID
• Obtaining an billing ID
• Obtaining other parameters from a database and checking to see if those parameters had values.

This resulted in a method of  200+ lines. The clarity or intention of the method has been lost. To regain the intention of this method, the method should be functionally decomposed into smaller methods.

i.e.


Now the intention of this method is clearer. The code becomes self-describing and there is not need for extraneous comments.

'Programming by Intention' is not used to declare all commenting is bad, just that commenting must not duplicate a purpose. If the code is clear then commenting what the code does is unneeded. However comments may still be needed. You could draw attention to a particular algorithm being used i.e MergeSort or that the code fixes a particular defect. When the comment has value it should be included, if not it should be discarded.

Use of exceptions to control program flow

Simple. Don't to it. The following link provides arguments:

http://c2.com/cgi/wiki?DontUseExceptionsForFlowControl

Throwing of ambiguous exceptions

There were numerous occasions where java.lang.Exception was thrown instead of a more specific exception. This is bad practice because throwing an ambiguous exception means the catcher cannot react to the exception in different ways. An ambiguous exception loses information on whether the situation is recoverable or irrecoverable. The meaning of the error is also lost. A specific exception should be thrown for a particular situation.

http://c2.com/cgi/wiki?ExceptionPatterns

Use parameter objects instead of long method signatures.

There were a few cases where methods had long method signatures. I'm talking about 10 or more parameters. This makes the method call unwieldy and prone to mistakes. A better approach is to use a parameter object which encapsulates the method signature, simplifying the method call.

http://c2.com/cgi/wiki?ParameterObject

Furthermore different parameter objects can be used to group together related parameters for different contexts. This is preferable than nullifying unneeded parameters in the long method signature.

Never Duplicate Code

http://c2.com/cgi/wiki?OnceAndOnlyOnce

A few examples were observed where code was duplicated and in some places just a copy and paste job.  This means that if any defects are found, then the fixes have to be applied in more than one place. Ideally code should be written using DRY principles i.e. Don't Repeat Yourself. Situations where the same code exists in different places should be remedied by that code being pulled out into a separate method and re-used.

Return nulls from methods.

There were a few cases where a call to a method resulted in a null being returned. For instance a client asked for a map and got returned a null because the input parameters were incorrect. The onus is then on the callee to check the result is not null before using the result. A better approach would be to return a NULL object. The Null Object pattern provides an alternative. It connotes the absence of an object. Instead of using null, the Null Object pattern uses a reference to an object that doesn’t do anything.

http://en.wikipedia.org/wiki/Null_Object_pattern

In this example, instead of returning null, an empty map should be returned. The client then doesn't have to check for nulls. This leads to safer code.

Conclusion

A lot of my recommendations are based on Martin Fowlers' Refactoring which gives guidance on how to remove particular code smells. However as legacy code is usually not particularly amenable to unit testing, refactoring can give a low confidence level as there are not the number of unit tests to back it up. Part of the Test Driven Design (TDD) approach is that unit tests are written to prove the behaviour of the system at a granular level. Once you have the tests, you have the confidence to refactor as you can regression test to see the system behaves as before. In my opinion a lot of the defects seen in production code would be diminished by the use of unit testing and paying heed to the aforementioned anti-patterns.

Recommended Reading

• Refactoring: Improving the Design of Existing Code by Martin Fowler. The classic book on taking poorly designed code and making it better without breaking it.
• A good introduction to TDD. Test-Driven Development: A Practical Guide by Dave Astels
• A TDD book for the more seasoned developer: Growing Object-Orientated Software, Guided by Tests by Steve Freeman and Nat Pryce. Probably if TDD was adopted in the first place many of these problems I described would not have arisen in the first place.
• But if you have a legacy code base showing these symptoms then a great book to read is Working Effectively with Legacy Code by Martin Feather. It has a set of techniques for changing code so that it is more suitable to be covered by unit tests

Musings on Behaviour Driven Development

I've been following Behaviour Driven Development (BDD) from a distance, for a while and was reading a couple of good articles about it. A good introduction is one by Dan North.

http://dannorth.net/introducing-bdd/

As an advocate of Test Driven Development (TDD), I find sometimes it can make you focus on the finer detail at the expense of missing the bigger picture. Hopefully BDD can be used to fill this gap.
I'm all for closing the loop between QA/business analysts and developers. Using traditional approaches, it's inevitable that some things will be lost in translation.

I've had a brief look at Cucumber and conceptually I like what I see. However as my main skill set is in Java, I wasn't too enamoured in having to learn Ruby to get BDD benefits. Fortunately there are options. I like JBehave's approach to BDD. It's more amenable in the sense that the stories(specifications) are written in simple English and steps are written in Java in preference to Ruby.

A business analyst can write a number of stories in the normal BDD format in plan text files i.e. Given X, When Y Then X

For example:

• Given a refund request with a threshold of 10.0
• When refund request received for 20.0
• Then the alert status should be ON

Every step (Given, When, Then) is then executed by a JUnit test which can extract parameters from the step. So basically the acceptance criteria are always driven by the business analyst. There is little opportunity for ambiguity as a tight coupling will always be enforced between the specifications and its JUnit counterpart.

See http://jbehave.org/reference/stable/developing-stories.html for more information and examples.

I expect that the path to getting comfortable with BDD will be similar to TDD: writing lots of tests, some of them fairly bad, until over time we get a certain feeling for what's right or wrong develops.

My only reservation is that most of examples I've seen are relatively simple. I'd be interested in a real-life example especially in the potential complexity of stories.

I am sceptical but at the same time curious. But I see that by utilising a BDD approach the test fixtures and tests become self describing.  They exhibit meta data for the understanding of  the intentions and actions of the code. Anyone who's worked with me knows that documentation is the bane of my life. Anything that could make my life easier in that respect gets a thumbs up from me :).

Miles Davis The Original ScrumMaster?

2009 was the 50th birthday of Miles Davis' Kind of Blue, a Jazz classic, indeed a classic for music of any time.

Listening to Kind of Blue again, got me thinking. Those recording sessions embodied everything good that should be part of Scrum.

• Communication - Musicians tend to be a synergistic bunch anyway especially Jazz musicians. . If you get a chance to watch a Jazz group in action, forget the music. Watch how they communicate. They're constantly listening or watching each other to guide them forward.

• Empower the team - Although Miles lead the group, he didn't tell everyone exactly what to do but let the group (or team) get on with it. He trusted them. In fact most of the tunes weren't written out specifically. A few modal scales were written out and it was left to group to improvise.

• Give value early - Most of the tracks on Kind of Blue were first or second takes. If that's not giving value early, I don't know what is.

• Learn and improve. - It was interesting listening to the out-takes of the recording session. All players were responding to each other to improve each other's playing. A notable example is the recording of 'So What'. In one of the takes, the recording is stopped early because Miles did not like the beginning. When you hear the accepted take, you'll agree that pick was the correct choice.

Each of these players changed music in so many different ways during the next decade, many of them becoming leaders of their own groups. They learnt from this session and changed the course of music forever. It truly was a sum of parts is greater than the whole.


And that's why Miles would be a great scrum master, even if he didn't know it :).

Some programming nuggets

I recently came across this list of nuggets that new programmers should learn and veterans have learned the hard way :).

Check out the link:

Things every programmer should know

Upgrading ReadyNas Duo

The time had come to upgrade the hard drives in my NAS. 500 GB in a RAID configuration is not much space these days. I looked around for a good 2TB drive with good eco credentials and quietness.

I finally plumped for a couple of Western Digital WD20EADS Caviar Green hard drives.

Drives not recognized

I place the new drive into the NAS but the WD20EADS was not recognized. I felt deflated; maybe I had bought two very expensive paperweights. A check on the ReadyNas DUO forums informed me that I should upgrade to a newer version of Radiator 4.17

Apparently these drives are not recognized by Radiator 4.16. I've installed the new firmware and voilà my new drive was then recognized. Unfortunately, I noticed another problem.

Ever increasing LCC count

Although these drives are eco-friendly there not very amiable with Unix-like systems. Seems that these drives are too clever for their own good. To be more green, these drives park their heads if there is no disk activity after 8 seconds. But Unix will usually frequently write to a disk on a periodic basis, which means the heads will continually be parked and unparked. This increase Load cycle count, LCC unnecessarily. This is important as there is an upper limit to the LCC count, say 300,000 after which the drive will shutdown due to it thinking there is a drive error. You'll then be unable to access your data and will have to send the drive back to Western Digital for replacement under warranty. Not good :(.

To get around this there is a WD utility, wdidle3, which allows the changing of the parking frequency or to disable it all together. This is a DOS application so it needs to be run from a boot disk. You can't run it from Windows 7 x64 either as it's a 16 bit application. What I did was plugged in the drives into my PC and created a USB disk to boot into DOS after which I could run wdidle3.

I followed these instructions:

1. Download FDOEMCD.builder.zip from http://www.fdos.org/bootdisks/.
2. Add wdidle3.exe to CDROOT folder. (wdidle3.exe)
3. Execute MAKEISO.BAT to create new FDOEM.ISO CD image file.
4. Burn FDOEM.ISO to CD or usb stick. It will boot to DOS and allows wdidle3 to run.
5. Disable wdidle3 timer on all discs (wdidle3 /D) or increased the park time to max time of 5 minutes (wdidle3 /S300).
   

More in depth Instructions found here: http://forum.synology.com/enu/viewtopic.php?f=124&t=20907

I set the park time to 5 minutes rather than disable it. This reduces the risk of platter errors should the drives be knocked. If you disable, the heads will always be in contact with the platters so if you inadvertently knock your hard drive you could damage it.

Another thing is that your drive may not be recognized from DOS. This is probably a BIOS issue. Check that your hard drive controllers are not using AHCPI. Change them to use IDE for wdidle3 update and revert them to AHCPI afterwards. It shouldn't take long for wdidle3 to find your SATA drive. If it longer than 10 seconds, then check the BIOS.

Since I've made these changes my LCC count has hardly risen. To put it into context, without wdidle3, my LCC increased to around 1000 in just 1 hour which means my drive would have been bricked in less than a fortnight. But now everything's hunky-dory.

Western Digital needs to get their act together. They really shouldn't be selling these drives without warning consumers that you may have problems on non-desktop computers. You live and learn.

Thoughts on ReadyNas Duo

I've been looking for a new NAS for a while. I needed to back up my current NAS, a DLink DNS-323 before I upgraded the firmware. I was initially going to buy just an external USB drive when I came across an offer from Netgear who are offering a free 500gb drive for customers who buy a ReadyNas Duo.

http://www.netgear.co.uk/free500gbdrive.php

BroadbandBuyer were selling ReadyNas Duos for £150 for a bare-boned system. Sounded liked a good offer so I went for it. I got the drive on a Wednesday, sent a copy of invoice and box serial number to Netgear and a week a later I had a free 500gb drive. I was presently surprised that they sent a Seagate ST3500418AS which retails for around £40. I was expecting a cheap hard drive and not a reputable brand. They have been some issues with this drive in the past but I was sent a 12th generation drive where all the problems have been ironed out due to the latest firmware.

Before I got the new drive I tried to access the ReadyNas bare-boned. It turns out this can't be done. I was hoping to use the NAS simply as a hub in the interim for some other USB drives I have hanging round so I could access them from the network but no dice. This is because when you insert a new drive, it installs Linux to the root partition and then mounts the remaining space as a separate drive for your data. So without a drive you have no OS. Pity as I could this with the DNS-323. No biggie.

It's very easy to add a new drive to the ReadyNas Duo. The NAS can be opened from the front, where the disk caddies are located. You can put in a new drive in less than a minute. When you first turn on the ReadyNas, the fan comes on at full strength and is quite loud. I was initially disappointed. There's no way I could put this NAS in the lounge with all that noise. Fortunately the noise is only temporary. The fan calibrates itself when the NAS is turned on and settles down to unnoticeable hum after a few minutes.

The ReadyNas OS is a version of Linux so it's customizable. There are a number of add-ons which can download from the ReadyNas-Addons site. Unfortunately out of the box you can't access the shell. If you're of the inquisitive sort, the first add-ons to download should be:

• ToggleSSH - As name implies, it enables SSH support
• EnableRootSSH - Enable the root user so you can SSH in as root.
• Apt - Enables Debian APT software packages.

When Apt is installed, you should change the /etc/apt/sources.list to get some more software. I've listed the sources.list I'm using. The entries after #Added are the items that I've added.

#Default
deb http://www.readynas.com/packages readynas/
deb http://archive.debian.org/debian sarge main contrib non-free
deb-src http://archive.debian.org/debian sarge main contrib non-free

#Added
deb http://archive.debian.org/debian-security sarge/updates main contrib non-free
deb http://archive.debian.org/backports.org sarge-backports main contrib non-free

deb-src http://archive.debian.org/debian-security sarge/updates main contrib non-free
deb-src http://archive.debian.org/backports.org sarge-backports main contrib non-free

I'm a vim person so the first thing after I installed APT was to install vim. I intend to use the ReadyNas as a Version Control System (VCS) and was happy to discover that Subversion and Git are both available from APT.

Caveat Emptor

When I buy a NAS, I like to think that I can still access the data if the NAS dies. I can just eject the disk and use in a USB drive enclosure so I can access the data on a PC. Unfortunately this is not the case. The main problem is that the ReadyNas uses a Sparc processor and by default formats the drive as EXT3 with 16K blocks. This 16k block size cannot be mounted on any x86 computer (or not very easily!). So unless you got a Sparc or Itanium-machine (which can mount it), data recovery on a PC is not an option. As Homer Simpson would say, DOH!!. Fortunately there is a solution if you happy to get your hands dirty with a bit of Linux. I found a solution on the ReadyNAS forums. Basically this boils down to unmounting the drive partition, reformating it as an ext3 partition using 4K blocks, and then remounting the partition. More in depth instructions can be found here.

I should note that this action may invalidate your warranty so you should perform this only if you happy customizing the NAS and you want access to the data outside of the NAS. Reformatting took around 10 mins. It's best to do this when the drive is empty else you'll need to copy all your data somewhere else and then copy back once the drive has been reformatted. I was lucky as I was using a new empty drive. Netgear should really inform consumers of this issue. There's been a brouhaha on the forums about this issue. Netgear should act really fast and include the option to change block size into the factory-reset-functionality.

Backup of my original NAS

Now I needed to backup my original NAS. I thought as a person who knows Linux quite well, I could use rsync. The ReadyNas runs a bash shell and rsync and smbmount are available. From the command-line I mounted my DNS-323 and then started an rsync process to synchronize my old NAS contents to the ReadyNAS duo over the network. It's good that the option is there to tinker at the command-line level if needed although this is not recommended for newbies. What I found was that rsync is incredibly slow. I was getting a throughput of around 2gb an hour which means it would have taken 10 days for a complete backup. This may be to all the checksumming rsync does but even so I wasn't expecting that lack of performance. I also found that ReadyNAS locked up after a day of syncing and I had to pull the plug to get it to reboot. Dang!

Next I tried to configure a backup using the ReadyNAS web-based management application. It allows you to define backup jobs which can run on a schedule if required. I configured the ReadyNAS duo to sync the DNS-323 to a new folder on the ReadyNAS and kicked it off. This time the backup seem to be going a lot faster. I was getting around 10gb per hour backing up my old NAS over the network. As I'd now got access to the shell, I thought I'd see what processes are run during this backup.

nas-73-5F-21:/etc/apt# ps -ef | grep cp
root       975     1  0 Aug10 ?        00:00:00 udhcpc -i eth0 -H nas-73-5F-21 -n
root      2931     1  0 Aug10 ?        00:00:00 smbmount //192.168.23.35/Volume_1 /job_001 -o username chrisk password XXXXXX uid XXXXXX gid 1002 codepage cp437
root      2951  2902 15 Aug10 ?        03:56:29 cp -vua /job_001//backup/. /dns-323-backup/backup
root      2963   753  0 21:54 pts/1    00:00:00 grep cp

As you can see cp is used in preference to rsync which accounts for the speed increase. You can also specify a backup to make a log of an errors as the backup progresses. I'll look into this when the backup has finished. I been running for just over a day and so far 250gb has been copied. Slow but fast enough for my purposes at the moment. I'll probably see how much faster it is to backup from an attached USB drive instead of copying over the network in future.

Conclusion

Overall I think I give the ReadyNas Duo around 8/10. It's very user-friendly and accessible for a newbie to use. I've not even used any of the other bundled features such as the BitTorrent client or ITunes server yet. I've only got one drive at the moment so I'm not using any of the RAID features but the ReadyNas Duo supports hot-swapping of drives so you gradually increase the storage capacity of your NAS without shutting it down. Quite cool. I've taken a couple of marks off due to the sluggish nature of the NAS. This is probably due to the processor which I gather is not the fastest kid on the block and the lack of memory which is only 256mb. Fortunately the memory can be upgraded relatively cheaply to 1gb, which I gather from some forum experiences greatly increases the performance.