A few weeks ago I came across a piece of code inside a DAO class with logic to instantiate new model objects and return them with null contents (new object) instead of just null itself. This is wrong. It is particularly wrong because the entire rest of the DAO classes in this application use the normal method of just returning whatever hibernate does, IE, null if not found. layer separation is important. Please keep such logic out of the DAOs and put it where it belongs above. Doing this precluded using the same DAO for other purposes without changes in several classes - yuck, spaghetti.
Another gem I found with the same author was a class with about 7 methods, one public, the rest private. The public one had an if else structure which delegated to the other methods. The return type was boolean. Upon inspecting these methods to see what they did and why true or false might be returned, I came across the following :
boolean ret = false; // Lots of code, some of which throw exceptions... ret = true; return ret;
Which can of course quickly be refactored to :
// Lots of code, some of which throw exceptions... return true;
Which in turn can quicly be refactored into :
// Lots of code, some of which throw exceptions...But, of course, with a return type of void, NOT boolean.
As it turns out, there was some reasoning to this exceedingly dodgy code. The boolean, urr, i mean, the true value (or exceptions) were being processed and converted into serialised JSON for a front end display tool. I still would not have done this, rather I would have used a return type and embedded the exception, message, and true/false value in that directly (such a return object already existed). If you were going to do this, comments were essential… there were none. Poor form.
Lastly, again, from the same author, we have the following method signature :
Employee getEmployee(String email, Integer offset) {
Which, at first, seems reasonable enough. On closer inspection it was apparent that it actually had a few side effects. What it really did was this :
Employee employee = employeeDAO.findByEmail(email); if (employee.getEmailAddress() == null) { employee.setEmailAddress(email); employee.setLanguage(languageBO.getLanguage()); } else { languageBO.setLanguage(employee.getLanguage()); } labelTextBO.setLanguage(employee.getLanguage()); session.setRequestedFor(employee); session.setSubmittedBy(employee); session.setTimeZoneOffset((offset != null) ? offset : 0); session.setLastLogin(employee.getLastLogin()); getEmployeeDetailsFromLDAP(employee); return employee;
So, of course, I changed the method name to this such that any potential user could instantly tell what it was going to do for them…
Employee getEmployeeAndPopulateSessionAsSideEffect(String email, Integer offset) {
Again, this is the sort of code that leads to mistakes by unsuspecting developers that come along later and wonder why it behaves so stangely, afterall, all they did was get an employee, right?
The astute among you will, by now, have noticed that the DAO which returned the fresh object instead of null was the EmployeeDAOImpl class as used in the EmployeeBOImpl. This project was, unfortunately, riddled with such messy dangerous code. We’ll leave the class and method naming for another day as that was appalling in places too.
It would appear that the colour scheme for Java doesn’t go at all well with my theme’s style. I’ll try to remedy this shortly if possible.