Friday, March 25, 2011

Coding conventions versus readability

There is a coding convention in most languages that says do not use literals in the code. Literals are constants line 0, 1, "India" etc. Things which have a constant value. The advice is to declare a static constant and use that in your code. The reasoning is that if you want to change the value later, you simply need to change it at one place, at the top. Makes a lot of sense.

However, there is one aspect that the conventions don't mention which is very important in the use of this convention. About the name of the static constants that are being used. Let's see an example. Let's say we want to use a constant for the number of hours in a day, a good name for this constant would be NUM_HOURS_DAY. I have seen some people name this constant TWENTY_FOUR! Now the sole purpose of going about this whole exercise is so that you can change the value of this constant easily later. If you name it TWENTY_FOUR, sure, you can change the value to 25 but suddenly, the code becomes very confusing.

This happens more for things like 0 and 1. I have seen people name a constant that should have been declared IS_VALID = 1 instead declare it as in ONE = 1. The whole point is lost.

Now, that brings me to how far we should go in using these conventions.

I have been working on pure Java/JDBC code recently. The idea was fast performance and minimal fuss. So, we went with this. One problem with this approach is you have to handwrite the SQL. So you are actually preparing strings that hold SQL commands and then preparing statements and then running them. Well, since we are pretty much not going to change the database being used in this application from MySQL to anything else until December 12, 2012 and after that anyway, it won't matter, it should be ok.

So, the question now arises, as to whether these SQL commands should be scattered about in the code or should we declare them all as static constants and use the constants? For example,

private static String QUERY_FETCH_ALL_BOOKS = "select * from book";

Yes, the purist in you would say. And I will agree with you, if only for a bit.

Now, let's say the query becomes a tad more complicated:

"select users.user_id, users.email, count(*), max(classified_ads.posted)
from users, classified_ads
where users.user_id = classified_ads.user_id and users.user_type = ? and users.status = ?
group by users.user_id, users.email
order by upper(users.email);"

Now by keeping this query as a constant and using only the constant in the code, you lose out on the specifics of the query. You have no clue on what the parameters are and you do not know what columns it returns. By intelligent naming, may be you can resolve this to some extent. But what advantages does this offer? It only makes things more complicated. The readability of the code is lost. The concept behind this convention is excellent. But it should be used where it makes sense.

I believe that conventions should be there to facilitate better coding and maintenance. They should not be rules written in stone to be used whatever happens. We should use our heads and decide where they should be used and where they should not.

2 comments:

Shravan Kumar. M said...

You are right Kamal and I agree with it completely! I have been bite by it couple of times.

Recently in our project we replaced using constants holding keys to messages in the messages file, with using direct keys. The reason being our client was comfortable not to have constants in this scenario and it was an unnecessary overhead. "The methodology followed was inherited from past projects without putting much thought to it!"

Another important observation I would like to share here about constants is, they should be declared and used in a/ per context and not in a generic file all the time. Also, try using "Java 5.0 Enums" where applicable, this is a good substitute for a group of related constants which provide abstraction and namespace apart from other benefits.

Kamal D Shah said...

I think many people do that - do what others have said and done without putting any thought into it. This can be really bad.