to be clear, i'm not talking about re-designing or re-architecting working code. the stuff i deal with on a daily basis is plain ol' broken, often because someone violated a design or architectural intent and took a short cut. yeah, maybe *that* bit of code works, but either something else broke or something based on it can't work.
this isn't a "purity of design" issue for me, it's shoddy workmanship resulting in more effort to get it working down the line. my philosophy is: do it right the first time.
This is how I work too. Like you, I'm not hung up on "No, no, the best way to do this is Design Pattern X as written by the authors of this textbook, just the way I learned in my High Level Software Architecture & Design course!" when it comes to the real world. I'm talking relatively little things, common sense things, that add up when you're working with a half-million lines of code.
Things like 10 classes that are almost identical and really, really should have inherited from a common base class. Maybe it started as two or three and the person didn't think it worth the bother to do an inheritance scheme, and found it quicker to hit Copy+Paste, and over the years they've just kept that up. Now I'm ready to add an 11th -- do I perpetuate the mess with another Copy+Paste, or do I take the extra time to go back and re-architect everything?
Or a bunch of classes that do inherit from a base class. The base class defines an Init() method, and most of the subclasses call the base class Init() before adding their own thing. Except for 3 classes which decided to be their own dog and copy/pasted the contents of the base class Init() into their own. Now I have to make changes in four places.
Or a magic number used in one place, a #define helpfully placed in another, and a totally different #define in yet another place, all referencing the same constant. That's THREE things to check now, in countless places, to change one value. Someone didn't do their homework and check to see if the value they wanted to define already existed elsewhere.
The rules I've (unofficially) adopted for doing my work:
- If I don't need to touch it, I won't.
- If I do need to touch it, and it meets the corporate coding standard, even if it's got a WTF? design element, I keep it as-is and follow the same "style" unless there's a good reason to go back and fix it all up. The rule of thumb is to be consistent, make it look like a single author wrote everything. Sometimes this perpetuates bad code, but we don't have time to refactor it or risk breaking the system with deadlines looming.
- If it is not up to the coding standard, I will bring the code I touch up to standard. (Sometimes this just means adding comments to describe functions, or removing a magic number.) This means you might have a class with 10 crappy-looking methods and 2 pretty ones, but over time little by little the code will start looking better.
- If I'm writing code from scratch, I will exceed the corporate coding standard: liberally commented, spaced, algorithms well described, architected as best I can from the way I learned in school. My goal is to make my peer code reviewers smile and say "Wow, that looks great and was easy to understand - I actually enjoy reviewing your code!" (Yeah, someone actually said that!)
We err on the side of simplicity, too. A trinary construct like:
return foo ? x : y;
is clever, but
if (foo)
return x;
else
return y;
Is a whole lot easier to read at a glance, and produces exactly the same code.
