Here is some code I see that sets off alarm bells clanging in my head:
Explicitly calling life-cycle methods of components that are outside your control.
Since that sounds oh-so-generic, let me illustrate by way of example.
Assume you are writing a Servlet
and your implementation doesn't care whether the request was a POST or a GET (in the current RESTful world, this should never be the case, but let's keep that aside for the purposes of this post). So, the following would appear to be a reasonable implementation:
public class MyServlet extends HttpServlet {
protected void doGet(HttpServletRequest req, HttpServletResponse resp) {
//Handle the request
}
protected void doPost(HttpServletRequest req, HttpServletResponse resp) {
doGet(req, resp); //Just call doGet
}
}
I would never do this though. This is because doGet
, doPost
and the other doXXX
methods are life-cycle methods of the Servlet
. I have no control over when and how the servlet engine calls them. I also have no control over what the engine does once these methods return. It is possible that the engine performs some house-keeping tasks once a doGet
returns, and that action could be different from what is done once a doPost
returns.
It is really a simple matter of extracting the "common functionality" between such life cycle methods into a method and calling that method.
public class MyServlet extends HttpServlet {
protected void doGet(HttpServletRequest req, HttpServletResponse resp) {
handleRequest(req, resp);
}
protected void doPost(HttpServletRequest req, HttpServletResponse resp) {
handleRequest(req, resp);
}
/**
* Method that extracts out the common functionality between GET and POST requests
*/
private void handleRequest(HttpServletRequest req, HttpServletResponse resp){
//Handle the request
}
}
What if the life cycle methods are in an Interface?
One opinion that came up in a discussion was that it is safe to call other life-cycle methods if they are in an interface
as opposed to a class
. Note that the above code sample extends javax.servlet.http.HttpServlet
which is an abstract class
as opposed to the base javax.servlet.Servlet
which is an interface
. This has come up specially while discussing callbacks.
I disagree with this opinion since it assumes that any book-keeping has to be performed by the same (possibly abstract
) class that denotes the life-cycle component. This is not true. Take an imaginary UI toolkit for example, which has a Pane
as a UI element. Now, assume an interface for handling interaction callbacks:
interface InteractionListener{
public void onClick();
public void onRightClick();
public void onLongClick();
public void onDoubleClick();
}
In this case again, assume that you want the same action to be performed on long-click and right-click. So, you might be tempted to do this:
Pane pane = // ...
pane.setInteractionListener(new InteractionListener(){
public void onClick(){
//Handle single-click
}
public void onRightClick(){
//Handle right-click
}
public void onLongClick(){
onRightClick();
}
public void onDoubleClick(){
//Handle double-click
}
});
This is still wrong. Yes- InteractionListener
is an interface and there is no way it can have a concrete method that might perform house-keeping tasks. However, you have no control over how the UI toolkit engine invokes the call-backs. You also have no control over what action the engine takes after your onXXXClick()
methods return. The solution, again is exactly the same as before: extract the common code into a method which you then invoke from both onLongClick()
and onRightClick()
.
Conclusion
Avoid calling one component life-cycle method from another, when the component in question is outside your control. This could lead to unpredictable behavior. Simply refactor the common behavior into a method and call that method from both life-cycle call-backs.
A few other instances where I have seen this sort of code:
Android's
SqliteOpenHelper
. In theonUpgrade()
method, you probably want to take a backup of data, drop the tables, add new columns, and then re-create the tables. The implementation often looks like this:public class MySqliteOpenHelper extends SqliteOpenHelper{
public void onCreate(SQLiteDatabase db){
//Use SQL CREATE TABLE statements to create the tables.
}
public void onUpgrade(SQLiteDatabase db, int oldVersion, int newVersion){
/*
* 1. Take backup of data
* 2. Drop tables
* 3. Add columns
*
* And finally:
*/
onCreate(db); //This is not right.
}
}The right way to do this would be:
public class CorrectSqliteOpenHelper extends SqliteOpenHelper{
public void onCreate(SQLiteDatabase db){
createDatabase(db);
}
public void onUpgrade(SQLiteDatabase db, int oldVersion, int newVersion){
/*
* 1. Take backup of data
* 2. Drop tables
* 3. Add columns
*
* And finally:
*/
createDatabase(db);
}
private void createDatabase(SQLiteDatabase db){
//Use SQL CREATE TABLE statements to create the tables.
}
}
I will re-visit this post and add more instances as and when I come across them.