CodeSOD: Knowledge Transfer

Lucio Crusca is a consultant with a nice little portfolio of customers he works with. One of those customers was also a consultancy, and their end customer had a problem. The end customer's only in-house developer, Tyrell, was leaving. He'd worked there for 8 years, and nobody else knew anything about his job, his code, or really what exactly he'd been doing for 8 years.

They had two weeks to do a knowledge transfer before Tyrell was out the door. There was no chance of on-boarding someone in that time, so they wanted a consultant who could essentially act as a walking, talking USB drive, simply holding all of Tyrell's knowledge until they could have a full-time developer.

As you can imagine, the two week brain-dump turned into a two week "documentation crunch" as pretty much nothing had any real documentation. That lead to comments like:

 /** * Parses a log file. This function looks for the string "ERR-001" in the log file * produced by the parser daemon (see project NetParserDaemon). * If found it returns the file contents with the string. Otherwise it * returns the file contents without it. * Known bug: it needs more optimizations to handle very big files. In the * meantime we manually restart the parser daemon from time to time, so the * log file doesn't grow too much. * @param log_file_name File name * @return ArrayList */ public static ArrayList parseLogFile(String log_file_name) { " }

Read that comment, read the signature, and tell me if you have any idea what that method does? Because trust me, after reading the implementation, it's not going to get any clearer.

 public static ArrayList parseLogFile(String log_file_name) { try { ArrayList result = new ArrayList(); File f = new File(log_file_name); FileInputStream s = new FileInputStream(log_file_name); InputStreamReader r = new InputStreamReader(s); BufferedReader r2 = new BufferedReader(r); String line = null; int retry = 1; while (r2.readLine() != null) { try { for (int i = 0; i < retry; i++) { line = r2.readLine(); result.add(line); } if (line.contains("ERR-001")) return result; } catch (OutOfMemoryError e) { System.gc(); line = "Retry"; retry = (int)(Math.random() * 10 + 10); } } } catch(Exception e) { ArrayList result = new ArrayList(); result.add("ERR-001: File not found"); return result; } finally { // always return the file contents, even when there are exceptions return parseLogFile(log_file_name); } }

There's just" so much going on here. First, this method must be dead code. The return in the finally block always trumps any other return in Java, which means it would call itself recursively until a stack overflow. Also, don't put returns in your finally block.

So it doesn't work, clearly hasn't been tested, and certainly can't be invoked.

But the core loop demonstrates its own bizarre logic. We retry reading within a for loop, by default though, we only do 1 retry. If and only if we encounter an out of memory exception, then we set the retry to a random value and repeat the loop. Oh, and don't forget to garbage collect first. If any other exception happens, we'll catch that and just return an ArrayList with "ERR-001: File not found" in it, which raises some questions about what on earth ERR-001 actually means to this application.

By the time the company actually hired on a full-time developer, Lucio had already forgotten most of the knowledge dump, and the rushed documentation and broken code meant that there really wasn't much knowledge to transfer to the new developer, beyond, "Delete it, destroy the backups, and start from scratch."

[Advertisement] BuildMaster allows you to create a self-service release management platform that allows different teams to manage their applications. Explore how!