Monday, June 22, 2009

Stop anticipating requirements

As developers, we want an easier life. We worry and fear of unstable client requirements. We think of any possible ideas the client might come up with, and implement hooks for them in our current code (as long as they are not too tough, and even that is relative to a developer's skill).

Half the time, we are right, and we felt good about saving ourselves a lot of hell by putting in the hooks early.

The other half, the requirements never come. But we still felt good because we WILL be saving ourselves a lot of hell by putting in the hooks early.

But in general? The extensions and hooks we introduced made the system more complex. Code does not read that naturally, hidden within implementations of Template Pattern, nested one within another. The runtime structure of an application is so remotely different from the static structure of an application (class diagrams), that documentation no longer seem to be useful. In fact, they seem to be talking about totally different applications! The growing unnecessary hooks only add to more documentation, which seemed to scare every other developer away except yourself. And we wonder why.

But what could be worse? Well, that the requirements for a new feature came. And it is totally in conflict with the hook we planned. Well, we could argue that the hook was not well designed, and some change would be necessary, and then we could wait for the 'real' requirements to come. So for now, we add a hook to the hook, happy that we have added even more flexibility. And while we are at it, we thought of another possible client requirement. So we add another hook too.

And the story goes on. Hooks on hooks on hooks. on hooks. Half of them probably would never be used.

So what exactly went wrong? Obviously, the problem is us.

We tried to act like we are the client. Except we are not. We are probably the worse candidate to act like the client, since in most cases we are developing for a problem domain of which we are neither expert, nor even remotely familiar with.

Which is why as developers, we should always keep our solution simple. A simple solution is a flexible solution. Add hooks only if the actual client requirement comes. The value added service we provide here is faster delivery of solution, instead of a monolith and extensible solution but would require months to add new features.

Remember, in most cases the clients are the domain experts, not us. They may not always know what they want, but they do know what they do day-to-day. Spend time with them, understand their need. Do not give in to their desires. And do not give them something that they do not need.

Monday, June 8, 2009

Towards Better Code: No side effects, shorter methods, exit early, and log every invocations

Recall earlier that I had mentioned that I learnt through logs of existing program/framework.

It's weird, but I cannot emphasize enough about the importance and need to log.

I have been using a rather new approach to logging. Well, not exactly to logging, but a new approach combining logging and coding style.

First off, I keep my methods short. For example, given the following algorithm:

public void login(final String username, final String password) {
  // authenticate
  // check if user container is active
  // check if user is active
}

It will most likely evolve into the following code:

public void login(final String username, final String password) {
  authenticate(username, password);
  verifyUserContainerIsActive(username);
  verifyUserIsActive(username);
}
void authenticate(final String username, final String password) {
  final boolean valid = checkUserCredentials(username, password);
  if (!valid) throw new AuthenticationException();
}
void verifyUserContainerIsActive(final String username) {
  final Container container = getUserContainer(username);
  if (container.isInactive()) throw new UserContainerInactiveException();
}
void verifyUserIsActive(final String username) {
  final User user = getUser(username);
  if (user.isInactive()) throw new UserInactiveException();
}

That's just a rough idea. Basically, each method is composed of many more methods, which serve to describe what each step do. Benefits? Eliminate the need to comment complicated algorithms. Comments run the highest risk of being out of sync with the actual code intent.

Also, notice the use of exceptions in the inner methods. I had started out with an alternative idea, that the inner methods would return true/false values, which would. It basically create the following nasty code block:

public void login(final String username, final String password) {
  if (isValidCredentials(username, password)) {
    if (isUserContainerIsActive(username)) {
      if (isUserIsActive(username)) return;
      throw new UserInactiveException();
    }
    throw new UserContainerInactiveException();
  }
  throw new AuthenticationException();
}

Of course, the better way is as follows:

public void login(final String username, final String password) {
  if (isNotValidCredentials(username, password)) throw new AuthenticationException();
  if (isUserContainerNotIsActive(username)) throw new UserContainerInactiveException();
  if (isUserIsNotActive(username)) throw new UserInactiveException();
}

Personally, I still felt that my first example up there produce a much more readable version of the algorithm, which communicates the intent much better.

But the basic idea/rule here is, exit early. What is most important is to communicate the basic intent of the code. The other possible cases are, well, exceptions (pun intended), and should not clutter the original intent of the code.

And finally, on to logging.

Now, I log all entry and exit of a method, as well as method arguments and method return values. Ideally this should be done with AOP, but I am not doing that yet.

What benefit does this logging bring me?

I can actually, from tracing the method entry logs, form a very clear view of how a request pass through the entire system. An example of the above log might be as follows:

enter: login {username:kentlai, password:xxx}
enter: authenticate {username:kentlai, password:xxx}
exit: authenticate
enter: verifyUserContainerIsActive {username: kentlai}
exit: verifyUserContainerIsActive
enter: verifyUserIsActive
exception caught: UserInactiveException

With shorter methods, logging every method invocations, I can trace the flow very accurately. Sure, there is an increase in the size of logs, but the return in such investment is easier debugging. Without even hooking up to the debugger.

With the logged method arguments, and knowing which method is causing a problem (from the logs), I can actually write a quick unit test to fix any potential problem found.

But there is one last magic elixer in this combination. No side effects.

Basically, each class should not have mutable state. The only mutable state should be from external storage (eg. database). And if a function need to use any values from such storage, they should, themselves be encapsulated into methods of their own, with the return values logged.

Why? This allow you to put together a complete picture, the context and environment of which a method was executed under. When every variable becomes known and can be supplied once again to a method during unit testing, a method can be guaranteed to work consistently and constantly as expected, which allows for more robust code.

Funny it took me so long to realize such principals.