| Tuesday, January 15, 2008 | |
| | | A Developer's Perspective | |
 | David Brady has been programming professionally for nearly 20 years, split about evenly between Win32/C++ and programming for the web in PHP, Python and Ruby. An avid proponent of agile methodologies, David is rabid about unit testing and delivering "stuff that works". He currently works as a software consultant in Salt Lake City, Utah. |
 Dishonest Programming
By David Brady
There's a good part of Computer Science that's like magic. Unfortunately there's a bad part of Computer Science that's like religion.— Hal Abelson I tend to view things differently than most people. This causes me grief in unusual places: today at the grocery store I recited my 11-digit "Fresh Values" card number to the cashier from memory, then stared right at the PIN pad, which was asking me to swipe my card, and attempted to hand my card to the cashier. A friend who was with me laughed at my simultaneous display of brilliance and retardation. I nodded and said, "I face a... unique set of challenges in life."
But sometimes my different views are really beneficial. My greatest strength, I think, is being able to give a team of really brilliant programmers a completely different way of thinking about a problem. Hal's comment triggered a return to a pattern of thinking I started having about a year ago, about how religion and morality are two different things... and about whether we can apply the notion of morality to programming.
I first surprised myself with this thought one day as I was pair programming with a brilliant but inexperienced developer. He was very clever and could think through extremely complicated designs and, while holding them firmly in his head, go implement them. As we were coding, he had a brilliant but deceptive idea. There is a well-known design pattern that has an obvious purpose and an implicit side effect; Ross wanted the reverse of the implicit side effect so he started writing the design pattern backwards. It was neat, it was clever, and I was certain that Ross could implement the code and get it working without bugs.
I also knew that anybody maintaining the code would never notice that the design pattern was implemented backwards until they modified it and it suddenly stopped working. I thought hard for a moment, and then quietly asked, "Ross, are we being dishonest here? Anybody reading this code will think you're doing this, but what you're really doing is that." I remembered this because it's the only argument I ever won with Ross without a fight. He stopped, leaned his head to one side and said, "Umm... yeah, you're right. Okay, how should we do this?" We went on to write some much better code that afternoon, some much more honest code.
How do you teach honesty in coding? I'm not really sure. Yesterday I reviewed some code that was dishonest from top to bottom. In our application's accounting subsystem, you can create subaccounts and then link inventory to those accounts instead of the default general accounts they start with. For example, there is a general account called "Revenue". For each service in your catalog, you can put its revenue into a different subaccount if you wish, so you could put all your Grand Canyon trips in a subaccount called "Grand Canyon Revenue". At the end of the year you can examine your revenue accounts and see which areas of your company are the most profitable.
So, there's a bug in our code. It turns out you can assign a trip to more than one subaccount at a time. You could accidentally put your Grand Canyon trips into "Grand Canyon Revenue" and "Hummer Safari Revenue", and while the accounting system would put all the revenue into only one of those accounts, you don't know which account the system will pick. So I asked two of my coders to fix this bug.
In my mind, this logic is really clear: before linking a charge item to a subaccount, you need to check to see if that charge item already has a link to a subacount of the same base account. (Every trip touches Accounts Receivable, Sales Tax Liability, and Revenue; you can override all three of these if you wish, so long as you don't try to override the same base account twice.) So the pseudocode for this is straight up:
// in event handler for onCreateLink:
if ( hasOverride(chargeItem, baseAccount) ) {
// already overridden. Warn user and ask if they want to replace override
} else {
createOverride(chargeItem, baseAccount);
}The code for hasOverride() would be maybe 10 lines long; createOverride() is 1 probably line of code wrapped in 4 lines of error handling.
What was submitted for review was about 30 lines long. Not terrible, but far enough off to warrant further inspection. The REAL warning sign was that the code didn't work and one of the developers who wrote it was really having trouble thinking through it to debug it. His partner had left for the day, so I sat and we began to review.
Problem number one, the event handler, which I would call onCreateLink, was called createLink. It's a simple difference, just one word. Is it important? You tell me: this function can exit normally, without throwing an exception or raising an error, without creating a link. The method doesn't create a link; the method creates a link IF it thinks it should. This is not createLink(). This is createLinkMaybe(), or perhaps createLinkIfYouFeelLikeIt().Honesty Tip #1: Name functions for what they really do. You've been taught to write foo munging code by drawing a box on the paper and labeling it fooMunger() and then writing the code. Okay, but when you're done, go back and read your code. Is it really fooMunging code, or does it also do something else? If you're spending 80% of your method checking error conditions, your method is clearly less concerned with munging foos than determining whether it should munge foos. Congratulations--you've written an event handler. Rename it to onMungeFoo() or handleFooMungeRequest() or whatever, and extract the code that ONLY munges foos to mungeFoo(). Number two. There was no hasOverride() method, but rather a function called containsSimilar() that took the list of account overrides already loaded in the UI. What the heck does "similar" mean? Looking at the code is a pile of compare methods, none of which actually check the charge item or base account. The author had determined that if you created the new account override without checking it, you could compare it to the already created overrides. If you found a similar one already created, you knew that the override was illegal.
Honesty Tip #2: Let your yea mean yea and your nil mean nil. Don't set up code to cleverly create side effects. EVERY effect is a side effect! Name your code for the intended side effect and document any other side effects. Don't name your code for the unintended effect, especially if you think you're cleverly creating an abstraction. (If the abstraction is really good AND other code will use it unchanged, you may create an abstracted method but you should still not use the abstraction in your other code. Write a wrapper method that explains your intended effect, like: bool isLegalOverride(Foo a, Bar b) { return !areSimilar(a, b); } Number three. Once containsSimilar() had approved the accountOverride, the override was added to the list by violating the Law of Demeter:
setupModel.getOverrides().add(accountOverride); setupModel was a systemwide object, and getOverrides() was a list that was publicly accessible. Do you know what that makes SetupModel.accountOverrides? A global variable. Now, there are exceptions to the rule "never use globals", but this is exactly the kind of global they were talking about when they made the rule in the first place.
Honesty Tip #3: Don't pee on my leg and tell me it's raining, and don't write a public accessor to a private member and tell me that member is still private. If an object is completely and utterly accessible from anywhere in the application, it's a global variable. I finally rejected the entirety of the code submitted by these developers, because once we cleared up the dishonesty in the code, it became clear that the timing and placement of the code was such that it was too late to stop and ask the user to confirm or abort the override sequence. In other words, they had been so cleverly writing code to construct side effects, that they had spent four hours writing code that did not do what the original objective requested.
I see programmers get rolling and they act like they're trying to carefully sneak up on the problem without letting the compiler know what's going on. Don't do that! Start by writing down what you want to accomplish. Now write high-level methods to state that purpose. As you move forward writing the code, always write code that clearly shows what it is you're trying to do. Don't make me decode your side effects to attempt to infer your purpose.
Honesty Tip #4: Don't try to trick the compiler into giving you what you want. You'll only trick your coworkers (and probably yourself). Tell the compiler what you want and write code that makes it give it to you. Deceptive code is immoral code. You CAN write moral code--it means simply being honest with yourself, with the compiler, and with your coworkers.
Any time you feel yourself being clever, ask yourself a key question: are you being deceptively simple, or simply deceptive?
Until next time,
David Brady
http://chalain.livejournal.com/ | | |
| | |
| | | most clicked this week from dzone.com |
Most-clicked links this week |
| |
|
| |
| | | A recap of
some of the most popular and active Javalobby.org
discussions this week. |
| |
| | | Product and
service announcements for Java developers. |
| |
|
Back to newsletter list
Make Applications More Valuable with Caché
Perspective
