Article 43SE5 A Big Change

A Big Change

by
Remy Porter
from The Daily WTF on (#43SE5)

Circa 2009, Marylou took a position at an e-commerce firm. It was a small company, which had done its startup phase right in the midst of the DotCom Crash, but somehow made it out the other end with a steady revenue stream.

That itself turned out to be a problem, as once you turn a profit, investors stop investing. The company founders spent the next few years looking for investment to expand, and when they finally got it, they went on a hiring spree. That's where Marylou came in.

The company had been founded by developers, but over the years, they took on more and more leadership responsibilities, and handed off the development responsibilities to Joel. Joel was Marylou's new boss.

"So, the first thing you'll want to do is install our CVS client and our Eclipse plugin," Joel explained, working with Marylou to get her development box set up.

Marylou reached for the Eclipse plugin manager and started searching for a CVS plugin.

"No, no," Joel said. "Our CVS client and plugin. I forked CVS so we can use a customized version."

Marylou paused, waiting for the punchline. Joel took this silence as an opportunity to steal the mouse and start clicking around on her machine for her. "Y'see, we needed a system that let us easily stage releases before going into production. So I built one."

"Um" most organizations use a separate tool that sits on top of source control for that."

"Yeah, but this way, it's integrated!"

Once Marylou was set up, and had the code pulled to her machine, Joel started showing off some of his "clever" solutions. "Like, I wrote this using recursion!"

while(someMethod()) {// Do bunch of stuff}

"That" that isn't recursion," Marylou said.

"Well, it's like recursion."

"Right. So" what should I start working on?"

Joel pointed her to a configuration tool he'd built. Instead of building an admin page into the web UI ("That'd be too risky to expose to the Internet!"), he'd written a Java Swing GUI which connected directly to the database. It was a mess of tabs, most of which looked basically identical, did basically the same thing, but were just different enough to absolutely confuse and befuddle any user.

"So, we just need to change the behavior a little bit when someone clicks the save button," Joel explained. The change was a small alteration to one of the verification rules.

Marylou made the change that afternoon, and fired up the application to test. She quickly discovered that she hadn't made the change she thought she had. During Joel's tour of the code and highlights of the bits she'd need to change, she had gotten the absurd idea that all these panels shared a common codebase and inherited from a base class. That, of course, was a crazy idea. Joel had written the code to power one panel, and then copy/pasted it into the next panel, with minor changes. Rinse, repeat, and suddenly you've got a UI with 15 panels that all basically do the same thing but use copy/pasted variations on the code. To change the behavior of the save button required going to each and every panel and making the change. In total, there were 30,000 lines of code that were essentially duplicates of each other.

Marylou made the change, and got it set up through the broken CVS staging system for review. Then she went back, and looked through the duplicated panels. And then started refactoring. And refactoring. It took, in total, about a month of picking apart Joel's mess to build something that acted as a reusable widget with all the required features, which she kept plugging away at while working on all the other code. The 30KLoC shrunk to a "mere" 3KLoC. 15 unique widgets collapsed into one single class which could be dropped onto any screen. Future changes could easily be made.

It was a huge change, and it involved some serious changes to the class hierarchy. It was big enough that when Marylou shipped it off for Joel to code review, she expected it would take him awhile to look at, and would probably receive some pushback.

It was a reasonable expectation, but Joel had his own style. Minutes after she sent the request, Joel approved it, and filled in the required "Code Review Comment" field with "Looks great."

His glowing recommendation didn't help Marylou's confidence about the change, so she tracked down another peer, Chris, who had been hired in the same spree, but had a bit more industry experience. They sat down, went through the code together, and spent a few hours walking through the reasoning behind the changes and the actual implementation thereof. With Chris's seal of approval, Marylou felt much more confident in her changes.

This, of course, proves why Joel led the team. It took Marylou and Chris hours to decide that a massive refactoring "looks great," while Joel could tell at a glance. That's experience.

buildmaster-icon.png [Advertisement] Utilize BuildMaster to release your software with confidence, at the pace your business demands. Download today! TheDailyWtf?d=yIl2AUoC8zAIUe71_Jw_lE
External Content
Source RSS or Atom Feed
Feed Location http://syndication.thedailywtf.com/TheDailyWtf
Feed Title The Daily WTF
Feed Link http://thedailywtf.com/
Reply 0 comments