Are You Proud of Your Code? 682
An anonymous reader writes "I am downright embarrassed by the quality of my code. It is buggy, slow, fragile, and a nightmare to maintain. Do you feel the same way? If so, then what is holding you back from realizing your full potential? More importantly, what if anything are you planning to do about it? I enjoy programming and have from a young age (cut my teeth on BASIC on an Apple IIe). I have worked for companies large and small in a variety of languages and platforms. Sadly the one constant in my career is that I am assigned to projects that drift, seemingly aimlessly, from inception to a point where the client runs out of funding. Have any developers here successfully lobbied their company to stop or cut back on 'cowboy coding' and adopt best practices? Has anyone convinced their superiors that the customer isn't always right and saying no once in awhile is the best course of action?"
Getting better. (Score:5, Informative)
When looking back at my first project, I feel the same. But I also think that I've learned a lot from it, and all subsequent projects were much, much better.
So, by being "not proud" of your code, you've made the first step towards improving it.
More Design (Score:3, Informative)
Management (Score:3, Informative)
I know that with my own management, they're quite uninterested in quality - and entirely interested in the "schedule". They have a schedule, and want to meet it with our client (we're consultants) no matter what that does to our quality.
Of course, once the code is accepted by the client then by #definition it is good enough and changes to existing code are only possible if we can prove that the existing code is buggy.
Then there's the bizarre requirement that developers use copy/paste whenever possible. It's not as if we get paid by the line, but it seems that some of the senior architect types think that LOC matters. (no jokes please)
Add in management's desire to see as little change to things as possible, we get a very poor heurestic for Hill Climbing [wikipedia.org] as a model of our software development "practices".
Re:Getting better. (Score:4, Informative)
TCO (Score:3, Informative)
Saying "no" to the customer is not normally what's called for. Instead, your team must clearly state the total cost of any proposed change. Factoring maintainable quality into cost estimates is an art that an organization must learn if it wants to get asked to do another job after the money for this one runs out (project drift leads to no results leads to unhappy customers, as you well know). When the customer responds with "Well, isn't just as simple as changing X into Y?" then that's when you get to say "no."
Re:Something to note about other people's opinions (Score:5, Informative)
To be honest? (Score:3, Informative)
The 2 strongest defenses (Score:3, Informative)
There are two good ways to defend against shifting requirements.
First, keep in mind during design that changes will happen. They may be in 5 years or they may happen before the first line of code is written. If your design is modular and consistantly uses a decent internal API, many changes won't require a great deal of pain.
The second defense is the "change order". If you do not have a change order process in place, the customer will expect to make major revisions at the last minute without consequence. In turn, this will result in crappy code since the project will suddenly be late and go from profitable to a net loss.
OTOH, if you gracefully hear the customer out, then produce a change order for them to sign which outlines how much longer it will take and how much more it will cost, all is well. They'll either tell you to forget about the change or they'll approve it and the extra work is extra profit. There may be a few customers who will cancel the project in a huff, but that was their choice to make. At least you don't get the reputation for producing fragile bug ridden unmaintainable junk.
A useful rule of thumb there is that if you can accomplish the change more uickly than you can produce the change order, just do it to keep the customer happy. If that encourages them to bury you in trivial changes, you can always gather them up into a change order at that point and explain that the project is now too far along to trivially make those changes.
Sorry, I have to bite about this. (Score:5, Informative)
and I've met people who won't even look at code unless every single line is commented telling them precisely what it does, so "int i = a + 2;" has to have a comment above it saying "// create a signed 32-bit integer variable, i, and assign it two more than the value of a".
Why on earth would you write a comment like that? That is ridiculous. However, the line DOES need a comment. It's declaring a variable with a non-descriptive name and doing something specific with it. Now, the comment should do something like say what the variable is used for, if it's non-obvious. In this case, "int i" is usually used for loops, so it doesn't need a comment unless it's *not* used for loops.
int i = a + 2;
or
int i = a + 2;
writing comments like "initialise variable" is useless, but thinking that may be all there is shows a misunderstanding about how comments work.
You can assume the person reading your code is a programmer, familiar with the language used, and able to follow general program flow. However, he may not be familiar with the rest of the system, nor with any specific tricks you may use[1]. Comments should be like a director's commentary on the code, pointing out what may not be obvious, and giving the bigger picture - the reason why a specific variable is used a certain way, or what a messy few lines of code may be achieving, eg:
(horrible loop declaration here)
(even more horrible regex here)
(custom function calling here)
It's said comments are like sex - even when they're bad, they're still pretty good. I'm pretty sure I've never spent 3 hours trying to work out how to get THAT to work, though!
[1] eg, I use a relatively uncommon trick in Java doing string comparisons of if(!"blah".equals(myString)) because it can't fail on the null pointer check that if(!myString.equals("blah")) can. A simple example, but something more complicated will save someone time if it just has a quick comment next to it saying what that section of code is achieving.
Re:Something to note about other people's opinions (Score:3, Informative)
That said, I and my coworkers use includes in headers all the time. (ONLY include a header if you need the full definitions; if it's just a FooClass pointer, forward declarations are far better. And don't be afraid to split a header into "foo.h" (struct definitions" and "foo_types.h" (forward decls, typedefs, enums); most clients of the header only need the latter). Writing good code truly is an art form.
In 2 letters... (Score:3, Informative)
No.
Honestly, most of the time writing bad code is not faster than writing reasonably good code.
To improve even more, try having a policy of doing a quick code review before non trivial check-ins. Knowing that you'll have to show and explain your code to a peer, that does help in resisting the temptation to take some shortcuts (e.g., why save time writing when it will take longer to explain). Code reviews should be easy to "sell" to upper managers, as they provide a certain degree of mitigation for the risk of one programmer leaving.
Re:Ah, kids these days... (Score:2, Informative)
Re:Something to note about other people's opinions (Score:3, Informative)
The most important part is not in your boss but in you - if you want to be proud of your code, you have to start by taking pride in the readability of you code. Your ambition should not be to write code that is readable, but textbook quality: it should be suitable not just as an example to others, but should itself be instructive to others. Not hat you can always do that, but you should hold it as something you _want_ not something you 'ought to'.
Yes, crunch times and asshole bosses can make it necessaary to cut corners, but make it clear that this way of working insults you (not for aesthetic reasoons, but because it's wasteful ands stupid). But you can get away with a lot of best-practice work if you stay focused and don't start adding stuff you don't need. And it usually takes less than days for tidy code to pay off, so unless someone is peeking over you shoulder, you might not even have to defend it. _Never_ do anything "quick-and-dirty". There will never be an 'afterwards' in which to tidy up. If it's worth doing, it's worth doing properly. What you _can_ (and should) do is "quick-and-_simple_": cut features to the bone until you need them. Don't be tempted to make framework when what you need is a function. But be ready to refactor it out (to the germ of a framework) when a more general version is needed. It sounds like more work, but it usually isn't in reality, and there's less code to debug later, and you won't introduce bugs when one implementation changes and the other does not. This, again, has noticable effect even on the scale of days.
To the parent:
I agree that proper API comments are essntial, but faced between a well-commented but ugly API and a well-designed, well-named and consisten one with only minimal comments, I'm not sure I'd always take the well-documented one. Especially not if the code inside is clean enough to worthy an little look.
But when it comes to commenting internal code, a rally good habit is to rough out the general structure in a coarse pseudocode of TODO comments before coding, and then fill in the blanks, removing the TODO tag but leaving the text as a heading as each block is completed. And adding new todos as you think of stuff as you go along. That way the commenting becomes not a chore but a _really_ useful aid in you work, helping you think clearly and reminding you of context even when littel code is written yet. And of course, when it _saves_ work in stead of adding it, it becomes a lot more likely to get done, too.
This comment style goes very well with another good practice for readable code, that probably has another name but which I call "bailout" logic:
Use guards of various types to throw out pathological or skip-cases as early as possible (throw exceptions, use 'return', 'break', and 'continue') in stead of nesting everything that actually happens deep inside multilevel if/else/switches, or constructing complex or clever (a.k.a. bug-prone) condition statements.
This makes for a nice, 'story-like' read for the 'normal' or most intersting flow. It will often be a good idea to add a brief comment after such a bailout (or the block it is in), stating the precondition that has now been met by virtue of getting here.
Some people oppose this style, since you have exit points all over the place in stead of everything merging at the end. I will gladly trade that for making it _much_ easier to get a clear picture of what kind of state I'm in when I actually reach some interesting code. And you simply have less cases to worry about for most of the code.
Sometimes, if neither 'while', 'for' or 'do-while' _really_ map all that well to your actual loop logic, you can be a lot more by understandable with "while true", and then break or continue explicitly where they fit naturally in the sequence, in stead of forcing it to the start or end (or *shudder*, abusing the increment statement to do actual work)
Re:comments are important (Score:3, Informative)
I typically preface my more complex code blocks/functions etc with a brief algorithm this way someone in the future knows what I was intending to write and well and when I am writing I can verify that my code does what my algorithm says. This sort of documentation also helps clarify the task and helps define all possible logic scenarios. This way you can write a more complete, stable piece and possible bug free (yeah right) of software.
This is just a mature way of programming.
P.S. - I never claim someone else's code sucks just because they don't have my 'style' of coding. I just want the code to work properly.
Re:Something to note about other people's opinions (Score:4, Informative)
Suppose you have an exception raised in a given class. You know the line number, and you open the class... if it's involved in serving a purpose that's low-level enough, it's going to take you a far more time to figure out what it's for, how it's used, and what role it plays in the project as a whole if you don't have, say, 3 simple lines of plain English in the header comment.
Personally I don't write too many comments in my "normal" code. I write progressively more as the code gets more complicated -- some features are simply too complex to even fit the implementation in my head all at once, so I write a shorthand walk-through of the implementation before I start. When I'm done, sometimes the code has actually boiled down enough that I can chuck that (sometimes I'm not so lucky).
You have to comment implementations that "look wrong" or are counter-intuitive, possibly because of client demands ("the last software package did it that way, so our other systems are designed to correct for it..."). What, are you naming methods calculateScaleWronglyPerClientRequest()?
It can be wise to flag methods (or classes) that will *appear* unused to refactoring analysis, because they're loaded & accessed dynamically.
I could go on.
Re:That's Not Good Code (Score:3, Informative)
Though I'm sure you can design a particular application in which a square is behaviorally a rectangle, and therefore it would be ok to have square extend rectangle, it is not universally the case.
One of the best things I ever did was head over to Object Mentor's published articles [objectmentor.com] page and click on the Design Principles category. There you will find a paper on the Liskov Substitution Principle which defines exactly when it is appropriate for one class to extend another. (You won't go wrong reading all the papers in that Design Principles category—I found them riveting for hours on end.)
In this particular case, behaviorally speaking a rectangle has two degrees of configuration freedom whereas a square has one. So, if you think about the actual practice of defining a Rectangle API with get/setWidth() and get/setHeight() methods, you could then write a unit test against those methods to fix the proper behavior of a rectangle. One of those unit tests might go something like: {1} set width to 1, (2) set height to 2, (3) assert that getArea() returns 2. I challenge you to now implement the subclass of rectangle called square that will pass this unit test which behaviorally (and correctly) verifies rectangle-type behavior.
Your suggestion of making squareness an attribute of rectangle is interesting and one I have not yet heard in any of the instances I've presented this example. I imagine it would take the form of something like the method boolean Rectangle.isSquare(), and would return getHeight()==getWidth(). This would work, as long as clients to the rectangle API understood that its square-ness is temporally defined and could change in the next moment. This is, however, an important consideration, though. In making this a requirement of what it means to be a square, nearly all definition of the square is itself lost...the concept of a square becomes a temporal thing itself. The old mathematical standby of an object that forever and always has equal sides is no longer relevant to this implementation.