Beware Util Classes

Any class that has 'Util' in its name should be treated with suspicion. Util classes are what I call 'trash magnets'. Developers who are too lazy to figure out where a method really belongs are tempted to put all types of unrelated code into Util classes. Often these are static methods which increase coupling, making code brittle, and make classes difficult to test.

There are, of course, exceptions. A StringUtil class or DateUtil class that really only provides utilities for Strings and Dates is acceptable. But even those should be eyed with a bit of distrust as often the same functionality can be found in open source libraries that are more thoroughly thought out and tested. Utility classes almost always break the Single Responsibility Principle by breaking encapsulation. If you absolutely have to create a utility class, at least ensure that all the methods are related to a single concept.

Unfortunately, I often see utility classes that have totally unrelated methods grouped together. For instance, one would expect that a class called StrutsUtil would have utilities that are strictly related to the Apache Struts framework.Finding a method such as this in StrutsUtil is totally unacceptable:

public static long getMaximumWidgets()

Whatever Widget is, it is not a Struts concept. More than likely it is a business concept that belongs either in the domain layer or service layer. Something like StrutsUtil belongs strictly in the UI tier.

Two years from now after the current developer has left the project, some executive may tell the current development team that they have a month to convert their Struts application to GWT or framework XYZ. The team should really have the expectation that they can easily jettison something called StrutsUtil, but to their surprise they find business logic there.

Or a business analyst comes along and tells the team that the downstream system can now handle ten times as many Widgets as before and that the system should be able to easily support that. By all rights the team should expect code related to this to be in something like Widget.java, WidgetService.java, etc.

I've found that worst offending utility classes are named after the application. You can bet that a class called the XyzSystemUtil that belongs to the XyzSystem is going to be full of all types of foulness.

So Little Code - So Many Issues

ResultSet rs = pstatement.executeQuery();

boolean flag = false;



if (rs.next()) {

   if ("Y".equalsIgnoreCase(rs.getString(1)))

       ;

   {

       flag = true;

   }

}



if (flag) {

   // do something

}

How many problems can you find in the above code? I can think of a few.

The egregious issue is the stray semicolon after the condition check. Add two more brackets and it should become obvious what the issue is.

ResultSet rs = pstatement.executeQuery();

boolean flag = false;



if (rs.next()) {

   if ("Y".equalsIgnoreCase(rs.getString(2))) {

       ;

   }

   {

       flag = true;

   }

}



if (flag) {

   // do something

}

The semicolon is treated as a no-op and the conditional check treats that statement as to what should be executed if the condition is true. Because of this flag will always be set to true. I'm assuming that the developer intended that the flag only be set to true if the condition is true, but that is not what is happening.

The second issue may not cause a problem right away, but could easily cause an issue if the SQL query is changed in the future. Relying on the order of columns brought back by a query makes the code less readable and therefore more prone to future errors. Lets assume that the original query is something like:

select EMP_ID, ACTIVE_FLAG  from employees;

If someone comes along in the future and decides that we also need to retrieve the department number like so:

select EMP_ID, DEPT_NO, ACTIVE_FLAG from employees;

and changes the query, but forgets to change the index on the rs.getString(2), then we are going to have a big issue.

Rather than using the column index, it is preferable to use the column name:

rs.getString("ACTIVE_FLAG");

Another big issue is that multiple rows could be returned by the ResultSet, but the code assumes that we are only interested in the first row.

The hard-coded "Y" and bad variable name of 'flag' are also bad practices, but not as bad as the above.