Thoughts on software.

There are times when a not so well designed application makes you wonder if rewriting it would be easier than maintaining it. Resist the temptation. Such a situation calls for a refactoring, and not a rewrite, because whatever code you have, ugly as it may be, atleast "works". Throwing away so many hours of developer work-time isn't a good idea. Also, the business aspects(read as your manager) would never allow you to go in for such a major overhaul.

A part of my last two weeks was spent on code refactoring. When I started off, my aim was to arrive at a particular final design. It doesn't work that way. Code refactoring proceeds step-by-step, eliminating one code smell at a time. As you go on, you might see an opportunity to apply a particular design pattern, and then you can do that. But make sure that the application is stable at all times. One-change-at-a-time seems to be the best way to go about it. And some quick testing is a good way to ensure that nothing has broken.

Code refactoring also gives you an opportunity to look at the code you wrote, and how it fits with the application. I had made some mistakes in mixing implementation details with the algorithm code, and my muddled thinking was obvious to me from the poor names that I had used all around. Here's an example:

int GetPosIndex(PosDim p)

Based on this function's return value, I would decide whether the position was full or empty(-1 for full, otherwise full with the return value pointing to the Positions' index in the datastructure).
I could change it to

bool IsPositionEmpty(PosDim positionInstance)

If these methods belong to a class Container, then

containerInstance.IsPositionEmpty(positionInstance)

makes for more readable code than

containerInstance.GetPosIndex(positionInstance)

Another option is to write

containerInstance.GetPositionStatus(positionInstance)

but then you will have to take different actions based on the return values.
This may seem trivial, but can help catch logical errors in the code. Your code should be as close as possible to the English you speak, for easy and fast maintenance.

Another area that concerns me is the null check. For example,
Consider a finite state machine, with all its events, states, transitions and actions specified as an xml file. Based on the user's input, the FSM should make transitions from one state to another.
The basic control flow goes as follows:

event=GetEvent();
lastState=getStateFromStateStack();
transition=lastState.getTransition(event);
action=transition.getAction();

Now what happens if there is no event detected corresponding to a particular user input?
What happens if event, as returned from GetEvent is null, and the same null checking logic begins to infest your code?

event=GetEvent();
lastState=getStateFromStateStack();
if(lastState!=null && event!=null)
transition=lastState.getTransition(event);
if(transition!=null)
action=transition.getAction();

This looks ugly. An NPE(null pointer exception) is waiting to happen somewhere down the line. So what so we do about this?

There are the following approaches to solve this problem:
Return a default object, with an attribute that other code can use to know your object is in a usable state or not.
Or throw a CustomException if the situation demands it.
But don't be afraid to return a null if it makes sense, for instance a Hashtable, if it finds nothing matching your key, must return null, no other way out.

In the above case, I created a new event "noEvent" which would be returned if no event was found. Then I configured the FSM so that on detecting this event, the transition returned would be of "ErrorTransition" type, and the action could then be an "ErrorAction", as per the FSM's configuration.
But what happens if
transition=lastState.getTransition(event);
returns null, because the FSM did not know which transition to make from our present state on encountering an event? This is more of an FSM configuration problem. Our poor FSM can't make a well informed decision now. So, we should throw a gentle message to the user informing him that the FSM couldn't find a transition.

This reminds me of an interesting comment I read on CodeProject, as part of an article on "Exception Handling Best Practices in .NET by Daniel Turini".

"External data is not reliable. It must be extensively checked. It doesn't matter if the data is coming from the registry, database, from a disk, from a socket, from a file you just wrote or from the keyboard. All external data should be checked and only then you can rely on it. All too often I see programs that trust configuration files because the programmer never thought that someone would edit the file and corrupt it."

Another mistake I noticed was the file writing class, which would first delete the output file, and then write it. Here are the same author's comments that woke me up to this problem:
"When you're saving data, situations can happen:
* Not enough security privileges
* The device isn't there
* There's not enough space
* The device has a physical fault
That's why compression programs create a temporary file and rename it after they're done, instead of changing the original one: if the disk (or even the software) fails for some reason, you won't lose your original data."

Also, there's the exception handling, that I still haven't refactored.
Here's a tip I that caught my eye:

"Don't clear the stack trace when re-throwing an exception
The wrong way:
try
{
// Some code that throws an exception
}
catch (Exception ex)
{
// some code that handles the exception
throw ex;
}

Why is this wrong? Because, when you examine the stack trace, the point of the exception will be the line of the "throw ex;", hiding the real error location.
try
{
// Some code that throws an exception
}
catch (Exception ex)
{
// some code that handles the exception
throw;
}

What has changed? Instead of "throw ex;", which will throw a new exception and clear the stack trace, we have simply "throw;". If you don't specify the exception, the throw statement will simply rethrow the very same exception the catch statement caught. This will keep your stack trace intact, but still allows you to put code in your catch blocks."

More on this when my refactoring is completed.