No Changes Please

A new codebase at a new job is a lot like a new relationship: everything's great until you really get to know each other. Just ask Bradley, who joined Javatechsoft Industries a few months ago. He was brought on to lend a hand with an overdue project. The pay was good, the job came with life insurance, and he had plenty of experience with Enterprise Java. It seemed like the perfect fit.

Specs came in, Bradley shipped code out, and their honeymoon was smooth sailing. The bad things crept up slowly, poking their heads out of the code in funny little ways that didn't seem like a big deal, they were kind of cute, until"

 273 warnings

"What's this?" Bradley had just compiled a module he hadn't seen before. There were a lot of warnings, sure, but they were all pretty straightforward:

 WARNING: Import xxx is never used. WARNING: Import yyy is never used. WARNING: Type SomeType is a raw type. References to generic type SomeType<E> should be parameterized

It looked like code written in a hurry. Bradley was more than happy to clean it up. He started by removing the unused imports, and committed his changes. He had left the module far behind by the time the project lead, Bill, caught up to him on instant message.

BILL: brad, hey, we need to revert your changes ok? plz dont make changes other than whats in specs

It was pedantic and poorly spelled, but Bradley understood where Bill was coming from. Still, he hoped that cleaning up the sloppy code was on the roadmap.

BRADLEY: Sorry about that. When's this code due to be fixed?
BILL: we need to run tests on all affected modules b4 we touch any code
BRADLEY: Even when we're just removing unused imports?
BILL: yup
BRADLEY: Okay, but I can run the unit tests for that module"
BILL: there are none. all code to be tested live!1!

No wonder they hadn't cleaned anything up. Bradley kept delivering the new functionality, but he itched to leave every codebase a little better than he found it. Finding himself with some extra time, he decided to get them off the ground with more unit tests. The first step was to mock out the data access layer. He found all beans inherited from an abstract base class, like so:

 class MyAbstractBean { private JDBCTemplate jdbcTemplate; }

That was a problem. They were using the database adapter object by explicitly naming its type, instead of using its interface, JDBCOperations. With the interface, he could have mocked out DB access in his tests. He fired up the IM to ask Bill about this.

BILL: whats jdbc operatoins?
BRADLEY: It's the interface that JDBCTemplate implements. By switching the declaration I can automate testing for this code
BILL: we dont have time for new stuff rite now
BRADLEY: It's not new. It's been in Java since 2.5"
BILL: anyway, we can't change the base class then we have to test everything in the code! just stick to the specs! PLZ!!!1!

Javatechsoft Industries had backed themselves into the classic testing trap: it was too risky to make changes, because they had no automated tests, but the only way to get automated test was to make changes. It was an anti-pattern Bradley had seen many times before, and he had to admit, he hadn't been hired to drag them kicking and screaming out of the hole they'd dug. He put his head down, and focused on writing the best code he could while sticking to the specs. If he did that, he'd be safe" he thought.

His latest assignment was simple: implement a SOAP call to the service FooService, following these steps:

1. Open a connection. If an error results, return -1.
2. Attempt to send the message. If an error results, return -2.
3. Close connection.
4. Open connection. If an error results, return -1.
5. Retrieve response. If an error results, return -4.
6. Close connection.
7. Attempt to parse the XML response. If an error results, return -5.

The functions for each step already existed, so implementing this should have been a cakewalk. He pulled up the Javatechsoft Industries class:

class MySOAPHelper {Connection openConnection(...params ...) throws Exception { ... }void sendMessage(...params ...) throws Exception { ... }String retrieveMessage(...params...) throws Exception { ... }void closeConnection(...params ...) throws Exception { ... }}

"WHAT?" Bradley asked his monitor. The methods all threw Exception, with no customized type. He could still trap the errors and map them to the integer error codes requested, but it made for a lot more work. This time, he figured improving the codebase was in line with the spec:

Connection openConnection(...params ...) throws SOAPOpenConnectionException { ... }void sendMessage(...params ...) throws SOAPSendMessageException { ... }String retrieveMessage(...params...) throws SOAPRetrieveMessageException { ... }void closeConnection(...params ...) throws SOAPCloseConnectionException { ... }

With those custom Exception types, the rest of the assignment was easy. So easy, Bradley took an extra-long lunch. When he came back:

BILL: why did you change all these functoins?!? they affect 23% of the code! we cant change them! EVER!
BRADLEY: I could either write broken, fragile code, or they can throw typed exceptions. I could write a separate wrapper class?
BILL: NO. then u have 2 different ways of doing things. code should be CONSISTENT.

Only the instant messenger's lack of emoji support saved Bradley's job.

[Advertisement] BuildMaster is more than just an automation tool: it brings together the people, process, and practices that allow teams to deliver software rapidly, reliably, and responsibly. And it's incredibly easy to get started; download now and use the built-in tutorials and wizards to get your builds and/or deploys automated!