Clean Code: How important is it ?
I’m not inspired by many things to write a blog post, but something happened today that gave me the energy to write this post. As noted by the name of the blog, I love groovy. I’m loving Python more and more and even like C#, but I think Groovy is the ultimate for expressive code. I know that’s debatable, but some of Groovy’s benefits(compared to Java) simply are not debatable by reasonable people in my opinion. I recently suggested to some developers on my team to read the book “Clean Code” by Robert Martin. Maybe a week or two later, I sent a link to the Agile Principles @ http://agilemanifesto.org/principles.html. And I’ve been introducing groovy into our newer projects because I think it allows for coding tasks to be written cleaner and done in fewer lines of codes.
I often find cases where a developer is doing something completely unnecessary in code. Unnecessary especially if you think of the Groovy alternative which is available in the projects I’m referring to.
Let me know know what you think about these cases.
We have a Groovy class with several static methods, like this one, that do basic select, insert, delete, update statements. This project has no ORM because I don’t want the memory overhead. It executes an update statement and returns the number of rows modified, Groovy takes care of everything else.
Example 1:
def static insertMessage(String server, String jobName, String jobInstance, String logLevel, String message) {
def sql = """
INSERT INTO LOG_MESSAGE (
message_id,
server_name,
job_name,
job_instance,
log_level,
message,
created
) VALUES(
log_message_seq.nextval,
${server},
${jobName},
${jobInstance},
${logLevel},
${message},
${new Date().toTimestamp()}
)
"""
db.executeUpdate(sql);
}
A second developer comes along and writes this method in the same groovy class.
Example 2
public static int updateStatus(Long runInstanceId, Date completedTs, int threwException, int resultCode, String result) {
Connection conn = null;
int rowsUpdated = -1;
try {
conn = ConnectionFactory.getConnection(ConnectionFactory.INTERNAL_DATASOURCE_HANDLE);
Sql db = new Sql(conn);
String sql = """
update run_instance set completed_ts=$completedTs,
result_code=$resultCode,
result=$result,
exception_thrown=$threwException
where run_instance_id=$runInstanceId
"""
rowsUpdated = db.executeUpdate(sql);
}
finally {
JobUtil.close(conn);
}
return rowsUpdated;
}
So I asked the developer, which would you rather maintain ? They essentially do the exact same thing, but 1 has 2 statements and the other does the same thing just in many more statements. I sent out the agile manifesto because the principle, “Simplicity–the art of maximizing the amount of work not done–is essential.” is either not known, understood or appreciated. It’s like writing more code is good, all the time for some developers. It wasn’t a Groovy or Java question, both were Groovy. Example 2 in my opinion is less simple and has many unnecessary statements. Why write unnecessary code ? Which would you rather maintain ?
Here’s a different type of example. This method is called by a service class that is exposed via BlazeDS to a Flex client.
Example 3
public static List<Contact> getContacts(String jobName) {
def contacts = []
def sql = """
SELECT
contact_name contactName,
contact_type contactType
FROM
job_contacts
WHERE
job_name = ${jobName}
"""
db.eachRow(sql) {
contacts << new Contact(contactName: it.contactName, contactType: it.contactType);
}
contacts
}
Another developer comes along and writes this code(Example 4) in a Java class, rather than us than use Groovy. The developer notes that he doesn’t have to write Java Beans,
and this makes it a better option than the above example. He also notes there is no bean to maintain in ActionScript either. Also the SuperDao code that returns the list of maps was written specifically for this project, by said developer. Even though refactoring becomes painful, he maintains, no bean maintenance is worth it. What do you think ?
Example 4
public static List<Map> getInstancesByName(String group, String name) {
List<Map> list = new ArrayList<Map>();
String sql = "select * from RUN_INSTANCE where job_group=? and job_name=? order by fire_ts desc";
try {
SuperDao dao = ConnectionFactory.getDao(ConnectionFactory.INTERNAL_DATASOURCE_HANDLE);
list = dao.executeQuery(sql, jobGroup, jobName);
} catch (Exception e) {
log.error("SchedulerService.getJobInstancesByJobFullName() failed. message:" + e.getMessage(), e);
throw new DaoException(e);
}
return list;
}
Which would you rather maintain, Example 3 or Example 4 ?
Here’s more of the query results as a List of Maps.
Do you find this method clear, and would you prefer to maintain this or would you refactor to this to something else ?
Example 5
public static final Map getContactListAndSLAByJob(String jobGroup, String jobName) throws SQLException
{
AppDao dao= null;
Map contactsAndSLA=new HashMap();
try {
dao=DatabaseFactory.getDao(DatabaseFactory.INTERNAL_DATASOURCE_HANDLE);
String sql= "select * from contact order by contact_group, name";
contactsAndSLA.put("CONTACTS", dao.executeQuery(sql));
sql= "select * from SLA where job_group=? and job_name=?";
Map sla_=null;
List<Map> slaList = dao.executeQuery(sql, jobGroup, jobName);
if (slaList.size()>0)
{
sla=slaList.get(0);
Long slaId = (Long)sla.get("SLA_ID");
if (slaId!=null)
{
sql ="select email from SLA_CONTACT where sla_id = ? and sla_level=? order by email";
sla.put("WARN_CONTACTS", dao.executeQuery(sql, slaId,"WARN"));
sla.put("ERROR_CONTACTS", dao.executeQuery(sql, slaId,"ERROR"));
}
}
contactsAndSLA.put("SLA",sla);
}
finally {
JobUtil.close(dao);
}
LOGGER.debug("SLA and contacts = "+contactsAndSLA);
return contactsAndSLA;
}
Last one, what do you think of this ?
public void methoCall() {
JobExecutionResult result = null;
int isExceptionBol = OracleBoolean.FALSE;
int resultCode = JobExecutionResult.ERROR;
A single variable name is small issue, but it’s not when you put it in a huge method or follow a bad naming convention whenever you have this type of variable. So I asked a developer about this since we got on the topic of code clarity. Look at variable name “isExceptionalBol”. First off, wouldn’t you think this would be a Boolean given the name ? Secondly, isn’t having this variable name prefixed with “is” and end with “Bol” redundant, and lastly isn’t it just a bad variable name ? I mean do you know what this variable might be storing, i mean what exactly is true or false, or int ? The author of this fragment went on to say, variable names don’t matter if the code works. This is from one of our senior most java developers. Obviously, I don’t agree with that statement,what about you ?
class for the thread pool. It’s in the jdk so it has to be good I figured. Maybe it is, but I’m not there yet.
