Some Good Coding Practices
After you have spent a few years in the software world, have written a few hundreds of lines of code, have made a few mistakes, you sub-consciously begin to build a list of not-to-do things & the to-do things for up next coding task. This is not a list of things you can rattle off from memory after 5 glasses of beer, but once you are asked to do a source code review, this list lurks somewhere at the back of your mind, pointing out to you the possible problems like objects under a spot light. Many or all of these may also become a part of your code review guidelines, coding conventions document, or rules in a automated code review tool. Well, then, you come up with the idea of publishing this "list" so others may benefit from it & avoid making same mistakes & get a jump-start at being "better programmers. So here's what I am doing.
Like before, this is not a list that I can rattle off from memory, but items from the list keep surfacing every now & then. I will keep on adding to this list as and when these items surface in my mind (or more often when somebody is kind enough to offer me 5 glasses of beer!).
So here goes. Please feel free to share your experiences & comments.
Perils of "Double Dating"!
Yes, thats exactly what I mean. Lets take a look at this piece of code.
* Does a lot of crap.
* Sets the policy inforce dates & expiration dates.
public void calculateAndSavePolicyExpirationDate(PolicyBusinessObject pbo)
// Get the policy inforce date
Calendar calendar = pbo.getInforceDate();
// Do something...do something more ...blah...
// Now fire some rules to calculate number of days
int numberOfDays = figureOutNumberOfDays();
// Add number of days to the calendar...
// Set the expiration date of the policy
// Now save this object
// Cool, we are done! Another happy customer!
Great! A simple piece of code. Do you see anything wrong here?
On committing the transaction, when you check the database, you will find that the policy inforce date is now same as the expiration date! Does not sound right, does it?
What we did wrong here is that we used the same Calendar object returned by the call to getInforceDate() and added certain number of days to it and used it as the expiration date.
The reference returned by the getInforceDate() was the same object that we set as the expiration date. Result - both the inforce date & expiration date referred to the same Calendar object, and hence had the same value when the policy data was saved into the data store.
There are several ways to do this correctly. One of them would be to create a new Calendar object as the expiration date. Or you can use calendar.getTime() which returns a new instance of Date object everytime.
So next time, be careful when you are dealing with dates, or especially, when you are dealing with multiple dates at the same time!
Good ol' null check!
So you put a null check for every "suspicious" looking object that threatens to sneak into your code & give you a surprise, right when your boss is trying to find an excuse to yell at you. Great! You are a good progammer. What next? Well, there is more to the null check than just avoiding the null pointer exception.
There are few questions that you need to ask yourself, when you put a null check.
Is this object supposed to be null in the first place? May be you need to throw an exception if this object is null, in addition to inserting a null check (which the design document did not talk about).
If this object becomes NULL, what does it mean from a business perspective? The business impact of an object being null could be far reaching.
Eg: If an Account object is null, and the Account object maps to a table in database, it means the information (eg: Account Number) needed to build the Account object was not available to the method that was responsible for building it. If the information was not available to the 'builder' of this object, possibly the information was not accepted as input from user. If the information was not accepted as input, may be the GUI needs to have an additional input or the GUI flow needs to change, or...so on and so forth.
So the next time you put a (object != null) check, look out for the business impact of this null object. In other words, "Do mind your business".
If this object is being passed to your method as a parameter, and it turns out to be null, may be in this case your method was not supposed to be called at all! May be somebody else forgot to put a null check before calling your method. Somebody else needs to put a null check & throw an exception, instead of calling your method. It may not be your problem after all!
You dont miss this once-in-a-blue-moon chance to tell people that they did not do their job, do you?