Clean Code: Don’t mix different levels of abstractions

We spend more time on reading code than writing. So if the code is more readable then obviously it will increase the developer productivity.

Many people associate readability of code with coding conventions like following standard naming conventions, closing file, DB resources etc etc. When it comes to code reviews most of the people focus on these trivial things only, like checking for naming convention violations, properly releasing resources in finally block or not.

Do we need “Senior Resources” in team (I hate to call a human being as a Resource) to do these things?Tools like Findbugs, PMD, Checkstyle, Sonar can do that for you. I agree that following a standard naming convention by all the team members is a good thing. But that doesn’t increase the readability of code.

Let us take a simple example. I would like to implement Fund Transfer usecase and following are the rules to implement:

  • Source and target accounts should be valid accounts
  • Check whether source account has sufficient amount
  • Check whether source account has provision for Overdraft and check whether this transaction exceeds the overdraft limit
  • Check for duplicate transaction with last transaction. If source, target accounts and amount is same with last transaction consider it as a duplicate transaction
  • If everything is fine then transfer amount to target account

Assume we have the following implementation for the above usecase:

The above code is readable..right??…because:
1. We have followed naming conventions like camel casing variable names
2. We have all the open braces ({) on the method definition line
3. We have closed DB Connection in finally block
4. we have logged exception instead of using System.err.println()
and most important, it is working as expected.

So is it readable and clean code?? In my opinion absolutely not. There are many issues in the above code from readability perspective.
1. Mixing DB interaction code with business logic
2. Throwing IllegalArgumentException, RuntimeException etc from business methods instead of Business specific exceptions
3. Most importantly, the code is mixed with different levels of abstractions.

Let me explain what I mean by different levels of abstractions.

Firstly, from business perspective fund transfer means validating source/target accounts, checking for sufficient balance, checking for overdraft limit, checking for duplicate transaction and making the fund transfer.

From technical point of view there are various tasks like fetching Account details from DB, performing all the business related checks, throwing Exceptions if there are any violations, properly closing the resources etc.

But in the above code everything is mixed together.

While reading the code you start looking at JDBC code and your brain is working in Technical mode and after getting Account object from ResultSet you are checking for null and throwing Exception if it is null which is Business requirement. So immediately you need to switch your mind to Business mode and think “OK, if the account is invalid we want to abort the operation immediately”.

Though you managed to switch between Technical and Business modes, what about making an enhancement to one perticular subtask like “Fund transfer is considred duplicate only if it matches with the last transaction that happened with in an hour only”. To make that enhancement you have to go through the entire method because you haven’t modularised your sub-tasks and there is no separation of concerns.

Lets rewrite the above method as follows:

The above improved method do exactly what the initial verson is doing but now it looks lot better than earlier version.

  • We have divided the entire task into sub-tasks and implemented each sub-task in a separate method.
  • We have delegated DB interactions to DAOs
  • We are throwing Business specific Exceptions instead of Java language Exceptions
  • All in all we have separated the levels of abstractions.

At the first level we have highest level of abstraction in transferFunds(FundTransferTxn txn) method. By looking at this method we can understand what we are doing as part of Fund Transfer operation without worrying much about implementation or technical details.

At the second level we have business logic implementation methods checkForOverdraft, checkForDuplicateTransaction etc which performs business logic, again without worrying much about technical details.

At the lowest level we have technical implementation details in AccountDAO and TransactionDAO which contains DB interaction logic.

So the reader(future developer/maintainer) of your code can easily understand what you are doing at the high level and can dig into method which he is interested in.

As I said earlier, if we have to make the change to consider the transaction as a duplicate transaction only if it happened with in an hour, we can easily understand that checkForDuplicateTransaction() is the one we have to look into and make change.

Happy coding!!

Keep The Code Clean: WatchDog & SpotTheBug Approach

Before going to discuss “WatchDog & SpotTheBug Approach“, let me give a brief context on what is the needs for this.

Three months back I was asked to write core infrastructure code for our new application which uses all the latest and greatest technologies.
I have written the infrastructure code and implemented 2 usecases to demonstrate which logic should go into which layer and the code looks good(atleast to me :-)). Then I moved on to my main project and I was hearing that the project that i designed(from Now on-wards I will refer this as ProjectA) is going well.

After 3 months last week one of the developer of ProjectA came to me to help him in resolving some JAXB Marshalling issue. Then I imported the latest code into eclipse and started looking into the issue and I was literally shocked by looking at the messy code. First I resolved that issue and started looking into whole code and I was speechless. How come the code become such a mess in this short span of time, it is just 3 months.

  • There are Date Formatting methods in almost every Service class(Copy&Paste with different names)
  • There are Domain classes with 58 String properties and setters/getters. Customer class contains homeAddressLine1, homeAddressLine2, homeCity.., officeAddrLine1, officeAddrLine2, officeCity… There is no Address class.
  • In some classes XML to Java marshaling is done using JAXB and in some other classes using XStream and in some other places constructing XML string manually even though there is core utilities module with lots of XML marshaling utility methods.
  • In some classes SLF4J Logger is used and in some places Log4J Logger is being used.

and the list goes on…

So what just happend? Where is the problem?

We started this project by pledging to keep the code clean and highly maintainable/enhanceable. But now it is in worst possible state.

Somehow it is understandable if the code is legacy code and is messy because today’s latest way of doing things becomes tomorrow’s legacy and bad approach like externalizing the application configuration into XML was the way to go sometime back and now it became XML hell with shiny new Annotations. I am pretty sure that in a couple of years we will see “Get Rid of Annotation Hell by Using SomeNew Gr8 Way”. 

But in my case it is just 3 months old project.

When I think about the causes of why that code becomes such a mess I end-up with never-ending list of reasons:

  • Tight dead lines
  • Incompetent developers
  • Not using code quality checking tools
  • No code reviews
  • No time to clean the messy code

etc etc

So whatever the reason your code will become messy after sometime, especially when more number of people are working the project.

The worst part is you can’t blame anyone. Developer will say I have no time to cleanup the code as I have assigned high priority tasks. Tech Lead is busy in analysing and assigning the new tasks to developers.
Manager is busy in aggregating the team’s task status reports to satisfy his boss. Architect is busy in designing the new modules for new third party integration services. QA people are busy in preparing/executing their test cases for upcoming releases.

So whose responsibility it is to clean the code? Or in other way, How can we keep code clean even with all the above said Busy circumstances?

Before going to explain How “WatchDog & SpotTheBug Approach” works let me tell you another story.

3 years back I worked on a banking project which is well designed, well organised and well written code that I have ever seen so far. That project started almost 10 years back, but still the code quality is very good. How is it possible?

The only reason is If any developer check-in the code with some bad code like adding duplicate utility methods then within 4 hours that developer will recieve an email from a GUY asking for the explanation what is the need to add that method when that utility method is already available in core-utilities module. In case there is no valid reason, that developer has to open a new defect with “Cleaning Bad Code” in the defect title, assign the defect to himself and change the code and should check-in the files ASAP.

With this process, every team member in our team used to tripple check the code before checking into repository.

I think this is best possible way to keep the code clean. By now you may have clue on what I mean by “WatchDog”. Yes, I called the GUY as WatchDog. First of all, sorry for calling such an important role as Dog but it better describe what that guy will do. It will bark as soon as it saw some bad code.

Need for WatchDog:

As I mentioned above, everyone in the team might be busy with their high-priority tasks. They might not be able to spend time on cleaning the code. Also from the Business perspective Adding new customer-requested features might be high-priority than cleaning the code. Sometime even though Business know that in long run there is a chance that entire application becomes un-maintainable if they don’t cleanup the mess they will have to satisfy their customer first with some quick new features and will opt for short-term benefits.

We have plenty of Quality Checking tools like PMD, FindBugs, Sonar. But does these tools suggest to create an Address class instead of repeating all address properties for different type of addresses as i mentioned above. Does these tools suggest you to use same xml marshalling library across the project. As far as I know, they won’t.

So if you really want your software/product to sustain over time, I would suggest to hire a dedicated WatchDog(Human Being).

The WatchDog’s primary responsibilities would be:

  • Continuously checking for the code smells, duplicate methods, coding standards violations and send the report to entire team.
  • If possible point out the existing utility to use instead of creating duplicate methods.
  • Checking for design violations like establishing Database Connection or Transaction management code in wrong places(web layer for ex).
  • Checking for cyclic dependencies for between modules.
  • Exploring and suggesting well established, tested generic libraries like apache commons-*.jars, Google Guava instead of writing home grown solutions(I feel like instead of writing home grown Cache Management better to use Guava Cache,but YMMV)

So far so good if the WatchDog does its job well. What if the WatchDog itself is inefficient?? What if WatchDog is not Skilled enough to perform its job? Who is going to check whether WatchDog is doing good or not?  Here “SpotTheBug” program comes into picture.

“SpotTheBug”
I strongly believe in having a friendly culture to encourage the developers to come up with thoughts to better the software.

Every week each team member should come up with 3 points to better/clean the code. They can be: Bad code Identification, Better Design, New Features etc.

Instead of just saying that code is bad code, he has to specify why he is feeling that code is bad, how to rewrite it in better way and what would be the impact.

Based on the effectiveness of the points, value-points should be given to the developer and those points should definitely be considered in performance review(There should be some motivation right :-)).

With WatchDog and SpotTheBug programs in place, if the team can identify the bad code before the WatchDog caught it then it is going to be a negetive point for WatchDog. If WatchDog continuously getting negative points then it is time to evaluate the effectiveness of WatchDog itself.

By using this WatchDog & SpotTheBug approach combined with proper usage of Code Quality Checking Tools(FindBugs, PMD, Sonar) we can make sure the code is clean to the maximum extent.


Java Coding Best Practices: Better Search Implementation

In web applications searching for information based on the selected criteria and displaying the results is a very common requirement.
Suppose we need to search users based on their name. The end user will enter the username in the textbox and hit the search button and the user results will be fetched from database and display in a grid.
In the first look it looks simple and we start to implement it as follows:

public class UserSearchAction extends Action
{
public ActionForward execute(...)
{
SearchForm sf = (SearchForm)form;
String searchName = sf.getSearchName();
UserService userService = new UserService();
List<User> searchResults = userService.search(searchName);
//put search results in request and dsplay in JSP
}

}
public class UserService
{
public List<User> search(String username)
{
// query the DB and get the results by applying filter on USERNAME column
List<User> users = UserDAO.search(username);
}
}

The above implementation works fine for the current requirement.

Later client wants to display only 10 rows per page and display a message like “Displaying 1-10 of 35 Users”.

Now we need to change the above code for the change request.

public class UserSearchAction extends Action
{
public ActionForward execute(...)
{
SearchForm sf = (SearchForm)form;
String searchName = sf.getSearchName();
UserService userService = new UserService();
Map<String, Object> searchResultsMap = userService.search(searchName, start, pageSize);
List<User> users = (List<User>)searchResultsMap.get("DATA");
Integer count = (Integer)searchResultsMap.get("COUNT");
//put search results in request and dsplay in JSP
}

}
public class UserService
{
public Map<String, Object> search(String username, int start, int pageSize)
{
//Get the total number of results for this criteria
int count = UserDAO.searchResultsCount(username);
// query the DB and get the start to start+pageSize results by applying filter on USERNAME column
List<User> users = UserDAO.search(username, start, pageSize);
Map<String, Object> RESULTS_MAP = new HashMap<String, Object>();
RESULTS_MAP.put("DATA",users);
RESULTS_MAP.put("COUNT",count);
return RESULTS_MAP;
}
}

Later Client again wants to give an option to the end user to choose the search type either by UserID or by Username and show the paginated results.
Now we need to change the above code for the change request.

public class UserSearchAction extends Action
{
public ActionForward execute(...)
{
SearchForm sf = (SearchForm)form;
String searchName = sf.getSearchName();
String searchId = sf.getSearchId();
UserService userService = new UserService();
Map<String, Object> searchCriteriaMap = new HashMap<String, Object>();
//searchCriteriaMap.put("SEARCH_BY","NAME");
searchCriteriaMap.put("SEARCH_BY","ID");
searchCriteriaMap.put("ID",searchId);
searchCriteriaMap.put("START",start);
searchCriteriaMap.put("PAGESIZE",pageSize);

Map<String, Object> searchResultsMap = userService.search(searchCriteriaMap);
List<User> users = (List<User>)searchResultsMap.get("DATA");
Integer count = (Integer)searchResultsMap.get("COUNT");
//put search results in request and dsplay in JSP
}

}
public class UserService
{
public Map<String, Object> search(Map<String, Object> searchCriteriaMap)
{
return UserDAO.search(searchCriteriaMap);
}
}
public class UserDAO
{
public Map<String, Object> search(Map<String, Object> searchCriteriaMap)
{
String SEARCH_BY = (String)searchCriteriaMap.get("SEARCH_BY");
int start = (Integer)searchCriteriaMap.get("START");
int pageSize = (Integer)searchCriteriaMap.get("PAGESIZE");
if("ID".equals(SEARCH_BY))
{
int id = (Integer)searchCriteriaMap.get("ID");
//Get the total number of results for this criteria
int count = UserDAO.searchResultsCount(id);
// query the DB and get the start to start+pageSize results
//by applying filter on USER_ID column
List<User> users = search(id, start, pageSize);

}
else
{
String username = (String)searchCriteriaMap.get("USERNAME");
//Get the total number of results for this criteria
int count = UserDAO.searchResultsCount(username);
// query the DB and get the start to start+pageSize results
//by applying filter on USERNAME column
List<User> users = search(username, start, pageSize);

}
Map<String, Object> RESULTS_MAP = new HashMap<String, Object>();
RESULTS_MAP.put("DATA",users);
RESULTS_MAP.put("COUNT",count);
return RESULTS_MAP;
}

}

Finally the code became a big mess and completely violating the object oriented principles. There are lot of problems with the above code.
1. For each change request the method signatures are changing
2. Code needs to be changed for each enhancement like adding more search criteria

We can design a better object model for this kind of search functionality which is Object Oriented and enhancable as follws.

A generic SearchCriteria which holds common search criteria like pagination, sorting details.

package com.sivalabs.javabp;
public abstract class SearchCriteria
{
private boolean pagination = false;
private int pageSize = 25;
private String sortOrder = "ASC";

public boolean isPagination()
{
return pagination;
}
public void setPagination(boolean pagination)
{
this.pagination = pagination;
}
public String getSortOrder()
{
return sortOrder;
}
public void setSortOrder(String sortOrder)
{
this.sortOrder = sortOrder;
}
public int getPageSize()
{
return pageSize;
}
public void setPageSize(int pageSize)
{
this.pageSize = pageSize;
}

}

A generic SearchResults object which holds the actual results and other detials like total available results count, page wise results provider etc.

package com.sivalabs.javabp;

import java.util.ArrayList;
import java.util.List;

public abstract class SearchResults<T>
{
private int totalResults = 0;
private int pageSize = 25;
private List<T> results = null;

public int getPageSize()
{
return pageSize;
}
public void setPageSize(int pageSize)
{
this.pageSize = pageSize;
}
public int getTotalResults()
{
return totalResults;
}
private void setTotalResults(int totalResults)
{
this.totalResults = totalResults;
}

public List<T> getResults()
{
return results;
}
public List<T> getResults(int page)
{
if(page <= 0 || page > this.getNumberOfPages())
{
throw new RuntimeException
("Page number is zero or there are no that many page results.");
}
List<T> subList = new ArrayList<T>();
int start = (page -1)*this.getPageSize();
int end = start + this.getPageSize();
if(end > this.results.size())
{
end = this.results.size();
}
for (int i = start; i < end; i++)
{
subList.add(this.results.get(i));
}
return subList;
}

public int getNumberOfPages()
{
if(this.results == null || this.results.size() == 0)
{
return 0;
}
return (this.totalResults/this.pageSize)+(this.totalResults%this.pageSize > 0 ? 1: 0);
}
public void setResults(List<T> aRresults)
{
if(aRresults == null)
{
aRresults = new ArrayList<T>();
}
this.results = aRresults;
this.setTotalResults(this.results.size());
}

}

A SearchCriteria class specific to User Search.

package com.sivalabs.javabp;

public class UserSearchCriteria extends SearchCriteria
{
public enum UserSearchType
{
BY_ID, BY_NAME
};

private UserSearchType searchType = UserSearchType.BY_NAME;
private int id;
private String username;

public UserSearchType getSearchType()
{
return searchType;
}
public void setSearchType(UserSearchType searchType)
{
this.searchType = searchType;
}

public int getId()
{
return id;
}
public void setId(int id)
{
this.id = id;
}
public String getUsername()
{
return username;
}
public void setUsername(String username)
{
this.username = username;
}
}

A SearchResults class specific to User Search.

package com.sivalabs.javabp;
import java.text.MessageFormat;

public class UserSearchResults<T> extends SearchResults
{
public static String getDataGridMessage(int start, int end, int total)
{
return MessageFormat.format("Displaying {0} to {1} Users of {2}", start, end, total);
}

}

UserService takes the SearchCriteria, invokes the DAO and get the results, prepares the UserSearchResults and return it back.

package com.sivalabs.javabp;

import java.util.ArrayList;
import java.util.List;

import com.sivalabs.javabp.UserSearchCriteria.UserSearchType;
public class UserService
{
public SearchResults search(UserSearchCriteria searchCriteria)
{
UserSearchType searchType = searchCriteria.getSearchType();
String sortOrder = searchCriteria.getSortOrder();
System.out.println(searchType+":"+sortOrder);
List<User> results = null;
if(searchType == UserSearchType.BY_NAME)
{
//Use hibernate Criteria API to get and sort results
//based on USERNAME field in sortOrder
results = userDAO.searchUsers(...);
}
else if(searchType == UserSearchType.BY_ID)
{
//Use hibernate Criteria API to get and sort results
//based on USER_ID field in sortOrder
results = userDAO.searchUsers(...);
}

UserSearchResults searchResults = new UserSearchResults();
searchResults.setPageSize(searchCriteria.getPageSize());
searchResults.setResults(results);
return searchResults;
}

}
package com.sivalabs.javabp;
import com.sivalabs.javabp.UserSearchCriteria.UserSearchType;

public class TestClient
{
public static void main(String[] args)
{
UserSearchCriteria criteria = new UserSearchCriteria();
criteria.setPageSize(3);
//criteria.setSearchType(UserSearchType.BY_ID);
//criteria.setId(2);

criteria.setSearchType(UserSearchType.BY_NAME);
criteria.setUsername("s");

UserService userService = new UserService();
SearchResults searchResults = userService.search(criteria);

System.out.println(searchResults.getTotalResults());
System.out.println(searchResults.getResults().size()+":"+searchResults.getResults());
System.out.println(searchResults.getResults(1).size()+":"+searchResults.getResults(1));
}
}

With this approach if we want to add a new criteria like search by EMAIL we can do it as follows:
1. Add BY_EMAIL criteria type to UserSearchType enum
2. Add new property “email” to UserSearchCriteria
3. criteria.setSearchType(UserSearchType.BY_EMAIL);
criteria.setEmail(“gmail”);
4. In UserService prepare the HibernateCriteria with email filter.

Thats it 🙂

10 things to become an outstanding Java developer

If you are a java developer and passionate about technology, you can follow the below things which makes you an outstanding Java developer.

1. Have strong foundation and understanding on OO Principles
For a java developer having strong understanding on Object Oriented Programming is a must. Without having a strong foundation on OOPS, one can’t realize the beauty of an Object Oriented Programming language like Java. If you don’t have good idea on what OOPS is, eventhough you are using OOP language you may be still coding in procedural way.Just studying OO principles definitions won’t help much. we should know how to apply those OO principles in designing a solution in OO way. So one should have a strong knowledge on Object modeling, Inheritance, Polymorphism, Design Patterns.

2. Master the core APIs
It doesn’t matter how strong you are in terms of theoretical knowledge if you don’t know the language constructs and core APIs. In case of Java, one should have very strong hands on experience on core APIs like java.lang.*, I/O, Exceptions, Collections, Generics, Threads, JDBC etc. When it comes to Web application development, no matter which framework you are using having strong knowledge on Servlets, JSPs is a must.

3. Keep coding
Things look simpler when talking about them theoretically. We can give a solution to a problem very easily in theory. But we can realize the depth of the problem when we start implementing our approach. You will come to know the language limitations, or design best practices while coding. So keep coding.

4. Subscribe to forums
We are not alone. There are lots of people working on the same technologies that we are working on. When doing a simple proof of concept on a framework may not give you real challenges. But when you start using it on real projects you will face weird issues and you won’t find any solution in their official documentation. When started working on a new technology the best and first thing to do is subscribing to that technology forums. Whatever the issue you are facing someone else in this world might have already faced it earlier and might have found the solution. And it would be really really great if you can answer the questions asked by other forum users.

5. Follow blogs and respond
As I already told you are not alone. There are thousands of enthusiastic technology freaks around the world blogging their insights on technology. You can see different perspectives of same technology on blogs. Someone can find great features in a technology and someone else feels its a stupid framework giving his own reasons of why he felt like that. So you can see both good and bad of a technology on blogs. Follow the good blogs and respond/comment on posts with your opinion on that.

6. Read open source frameworks source code
A good developer will learn how to use a framework. But if you want to be an outstanding developer you should study the source code of various successful, popular frameworks where you can see the internal working mechanism of the framework and lot of best practices. It will help a lot in using the frameworks in very effective way.

7. Know the technology trends
In the open source software development technology trends keep on changing. By the time you get good idea on a framework that might become obsolete and some brand new framework came into picture with super-set of features. The problem which you are trying to solve with your current framework may be already solved by the new framework with a single line of configuration. So keep an eye on whats coming in and whats going out.

8. Keep repeatedly using code snippets/utilities handy
Overtime you may need to write/copy-paste same piece of code/configuration again and again. Keeping those kind of configuration snippets like log4.properties, jdbc configuration etc and utilities like StringUtils, ReflectionUtils, DBUtils will be more helpful. I know it itself don’t make you outstanding developer. But just imagine some co-developer came and ask you to help in fetching the list of values of a property from a collection of objects and then you just used your ReflectionUtil and gave the solution in few minutes. That will make you outstanding.

9. Know different development methodologies
Be familiar with various kinds of methodologies like Agile, SCRUM, XP, Waterfall etc. Nowadays choosing the development methodology is depending on the client. Some clients are preferring Agile and some clients are happy with waterfall model. So having an idea on various methodologies would be great.

10. Document/blog your thoughts on technology
In day to day job you may learn new things, new and better way of doing things, best practices, architectural ideas. Keep documenting those thoughts or blog it and share across the community. Imagine you solved a weird problem occurred while doing a simple POC and you blogged about it. May be some developer elsewhere in the world is facing the same issue on a production deployed application. Think how important that solution for that developer. So blog your thoughts, they might be helpful for others or to yourself.