Project Aardvark

Fixing Other People's Code

Monday, June 13, 2005 posted by Benjamin Pollack

It’s almost a truism that programmers would rather write new code than modify existing code. The truth of that adage is doubled when talking about modifying someone else’s existing code. Unfortunately, that’s exactly what Tyler and I have been doing nonstop for the past two weeks, and let me tell you, it has not always been pleasant.

After we got the source code the GPL component to compile in VS.NET, the next few days were spent, more than anything else, simply getting a feel for the code base. Different programmers have different conception of what orthogonal design entails; the degree of separation, the function of different components, the interfaces between classes, all vary dramatically from coder to coder and company to company, and invariably your conception is not quite the same as the fellow next to you. Understanding the design of the existing base, then, becomes very important to ensure that when you start making extensions, you do it right.

The problem is that when you’re first getting started, the chance of getting it truly right the first time—especially if you’re making large-scale modifications—is very low. In Aardvark, for example, I initially attached some new connection-specific handshaking code inside a class that controlled the windows taskbar icon for our program. That probably sounds a bit silly, but it actually made a reasonable amount of sense. The taskbar class was where connections were created and destroyed. Since that code was written for multiple connections, and I was restructuring it to allow only one connection, adding in some handshaking code into the new connection code there seemed as if it made sense at the time. It also had the benefit of keeping all Aardvark-centric modifications in just one class.

Needless to say, that was a bad call and caused problems down the road when Tyler started adding encryption code and needed to hook it in after the connection had been started and after Aardvark handshaking, but before any other communications took place. So that had to be refactored heavily. Thank heaven that Subversion lets you move files while preserving history.

It’s doubly a problem when two coders are trying to come to grips with the code base at the same time. While I was on one end of the room, adding handshaking code and modifying the interface, Tyler was on the other side of the room trying to make the connections use secure sockets. That meant we were each trying to modify the same section of code at once, and because we each had different concepts of what the original programmers had in mind from a style perspective, we stepped on each other’s toes a lot even with Subversion’s assistance to keep things straight. Despite our best efforts, checking in code that worked fine on one machine but that broke the other’s code base became a frequent occurrence.

(Thankfully, Subversion has numerous features—such as repository-wide versioning that lets you revert the entire source tree to the way it was at a specific point in time, powerful change tracking tools to figure out quickly which lines of code broke the program, and solid merge support—that made this far less painful than it could have been. And, of course, Subversion integrates very well with FogBUGZ, letting us track easily which check-ins fixed which bugs. Without Subversion, it would have taken us much longer to get here.)

Even if we had instantly made all the right decisions, though, stylistic variations would have hit us in another way. Large projects tend to have multiple standard styles for different components. The TightVNC project is a great example of this. vncviewer, which is the part of VNC that lets you control other machines, codes directly against the Winsock API in its ClientConnection class, whereas the sibling WinVNC server has a VSocket class that abstracts all socket communication and uses that class in vncServer and vncClient classes. WinVNC likewise makes extensive use of Windows’ Unicode string manipulation functions, whereas vncviewer codes against the ANSI functions. Classes in WinVNC are normally called vncFooBar or VNCFooBar, whereas classes in vncviewer are called just FooBar with no prefix. WinVNC makes extensive use of a pattern for dialog boxes that relies on threads; vncviewer lacks such a pattern.

This is not a problem in any real way; both code bases are solid, fairly consistent internally, and most importantly, as anyone who’s used TightVNC can tell you, very reliable. It’s just a bit confusing if you’re working in a project that has two fairly different styles to keep your coding appropriate, and the different high-level designs increases the chance that you’ll make a bad design decision by applying one project’s architecture to the other.

Would it be worth cleaning up? Almost certainly not. It would take a lot of time to get everything refactored to have a consistent style, and would come at no benefit whatsoever to the end-user. It does perhaps emphasize, though, that large projects (including Aardvark, thankfully) can benefit by having standard coding conventions to ease moving around the code base.