Journal Friends Archive Profile Memories Chalain Oct. 31st, 2006 06:07 pm Dishonest Programming 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 stupidity. 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 subaccount 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); } 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 theside 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:



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? Current Music: 171 BPM Playing to your Strength - DJ Steveboy

18 comments - Leave a comment From: howardtayler Date: October 31st, 2006 09:39 pm (UTC) (Link) I don't write code, and I get this.



I believe this means it is brilliant, and should be more widely published. Reply ) ( Thread From: darthparadox Date: October 31st, 2006 09:51 pm (UTC) (Link) And I write code for a living, and I get this too, and understand its implications for the work I do daily.



So, I'd have to agree. Reply ) ( Parent ) ( Thread From: davebennett Date: November 1st, 2006 04:43 am (UTC) (Link) I too must say this is a fine bit of writing, as well as good coding advice.



Now, if I only had an accounting system that was properly designed . . . unlike the one I deal with daily.



Why are all mid-market accounting/ERP systems huge kludge glops? Reply ) ( Thread From: reaverta Date: November 1st, 2006 07:44 am (UTC) (Link) ...Not only is it a fine bit of writing, and a fine bit of advice, but one I think I need to take to heart.



I keep thinking on the micro level, and frequently thinking sideways at that. This results in occasionally clever code, or code that, once it's explained, turns out to be highly cunning, and yet...



...Yeah, probably as dishonest as a politian on campaign. *cough* Reply ) ( Thread From: chalain Date: November 2nd, 2006 10:06 pm (UTC) (Link) I'm flattered and a little embarrassed that everybody is saying this is such good writing. I feel like it could use a lot of work, but... thank you. Reply ) ( Thread From: (Anonymous) Date: November 3rd, 2006 07:19 pm (UTC) Pingback from The Audio Fool (Link) http://blogs.msdn.com/audiofool/archive/2006/11/02/honesty-as-a-code-metric.aspx Reply ) ( Thread From: ksader Date: January 11th, 2008 03:29 pm (UTC) (Link) Awesome, I've del.icio.us linked it and shared it with my friends! Reply ) ( Thread From: chalain Date: January 11th, 2008 05:11 pm (UTC) (Link) Awesome, ksader! Thanks! Reply ) ( Parent ) ( Thread From: (Anonymous) Date: January 11th, 2008 06:56 pm (UTC) Nothing new here (Link) Agreed, your principles are right (supporting the value of 'communication'), but an old hat (only the names are 'new' and a bit of self marketing to me, maybe in order to make your post more interesting).



Your first two tips are known as the PIE principle - Program Intently and Expressively



Your third tip adresses the TDA principle - Tell Don't Ask



Your last tip is just about the KISS principle - Keep it simple, stupid (or sometimes known as Keep it simple and straightforward)



oh - and there's just another fine principle - it's called the DRY-principle: don't repeat yourself ... ;o) Reply ) ( Thread From: chalain Date: January 11th, 2008 07:41 pm (UTC) Re: Nothing new here (Link) I'm glad you can relate to distinct cognates from the points; I would argue that the integrity/honesty/morality angle is a single viewpoint that collects the four otherwise distinct principles.



More importantly, the viewpoint changes their perception. Your cognates are interesting to me, because all four of them are different ones than I was intending when I wrote the article. In other words, although you aren't wrong in what you have said, you have completely missed my point.



I agree that if you remove the integrity/honesty/morality angle, there is nothing new here. I will say, however, that not only do you miss the point by doing this, but you then misunderstand the specific details. For example, KISS and design by intent are quite unrelated, at least in my mind.



Maybe I communicated poorly. *shrug*; Either way, I am sorry we didn't connect. Thanks for taking the time to tell me I wasted your time. :-) Reply ) ( Parent ) ( Thread From: (Anonymous) Date: January 12th, 2008 12:42 pm (UTC) I don't get this (Link) >> don't write a public accessor to a private member and tell me that member is still private



So if you need to add an object to a list what is your proposal? Because I can see less problems in:



a.getList().add(obj)



than in creating a new method for every operation in the list interface (for each private collection in the class!). Like:



a.addObjectToList(obj)



The accessors are meant to wrap the field so logic can be attached to the operation but in the end they are making private fields public.



Am I missing something here? Reply ) ( Thread From: chalain Date: January 13th, 2008 01:37 am (UTC) Re: I don't get this (Link) I ended up writing a HUGE reply to this. It is here: http://chalain.livejournal.com/63878.html Reply ) ( Parent ) ( Thread From: (Anonymous) Date: January 13th, 2008 08:28 am (UTC) Poor naming is the most common offense (Link) We agree on naming -- poor choices for classes, methods, variables, you name it, is the most common design error I see.



On comments:

A well-respected colleague once said "comments are bad. The get out of date and mislead readers", or words to that effect. I had to chew on that for a couple of weeks before I decided that bad comments are bad. A good comment will describe the intended purpose of the commented code, and facilitate in determining if the code is a correct implementation or not. A good use for comments is to explain why something is done in what may appear to be a peculiar manner, or why a seemingly unnecessary piece of work is being done (e.g. to provide backward compatibility with legacy software). Reply ) ( Thread From: chalain Date: January 13th, 2008 08:57 pm (UTC) Re: Poor naming is the most common offense (Link)



I briefly held the belief that all comments were bad, but that doesn't hold up to careful scrutiny. I finally settled on "Comments are a sign that the code is bad."



A very good tool someone taught me for getting rid of comments near bad code is to convert the comment into a function. Consider:

// is ball out of bounds? if ( p.x < 0 || p.x > SCREEN_MAX_X || p.y < 0 || p.y > SCREEN_MAX_Y ) {...} versus

if (p.outOfBounds()) { ... } I'm not talking about documentation comments, here--things that explain what a -1 means to this function, or what the general algorithm is going to be, etc. But most of the comments I see are things saying "hold this magic value for later" or "these four methods initialize the subsystem", etc. I agree with most of your "a good use for comments" cases, with the possible exception of the unnecessary piece of work for legacy software. I would look hard at those on a case-by-case basis to see if I couldn't make that vanish into a function or object that handled the legacification for me. I would leave those commented only if I couldn't clean up the legacy entrails. Agreed.I briefly held the belief that all comments were bad, but that doesn't hold up to careful scrutiny. I finally settled on "Comments are a sign that the code is bad."good tool someone taught me for getting rid of comments near bad code is to convert the comment into a function. Consider:versusI'm not talking about documentation comments, here--things that explain what a -1to this function, or what the general algorithm is going to be, etc. But most of the comments I see are things saying "hold this magic value for later" or "these four methods initialize the subsystem", etc. I agree with most of your "a good use for comments" cases, with the possible exception of the unnecessary piece of work for legacy software. I would look hard at those on a case-by-case basis to see if I couldn't make that vanish into a function or object that handled the legacification for me. I would leave those commented only if I couldn't clean up the legacy entrails. Reply ) ( Parent ) ( Thread From: (Anonymous) Date: January 15th, 2008 10:56 am (UTC) (Link) What you describe is a good analogue for Rhetrorical Ethos. And I completely agree. Reply ) ( Thread From: (Anonymous) Date: January 16th, 2008 01:52 pm (UTC) Bullshit is a bigger problem than lying (Link) As Harry Frankfurt famously detailed in "On Bullshit" ( www.amazon.com/Bullshit-Harry-G-Frankfur t/dp/0691122946 ), a much bigger problem than lies is bullshit, and I find that is definitely the case with computer software development today.

I.E. a programmer has to know his/her stuff well enough to know that they are being dishonest. The problem is that most people being paid to manage/design/program/test/etc DON'T their stuff and are just making it up as they go along, sprinkling in a liberal dose of buzzwords. Oh, if we only all DID have C.S. degrees, understood them, and only needed to police our self exuberance!



www.polyglotinc.com Reply ) ( Thread From: chalain Date: January 16th, 2008 02:03 pm (UTC) Re: Bullshit is a bigger problem than lying (Link) A very good point! Interesting; I like your use of the term "bullshit" here and I feel like it applies exactly to what I am calling dishonesty here. Your term is a bit less pejorative, perhaps, for both good and ill. Bullshit may invoke less offense. In the context I first coined the term, dishonesty was a much more motivating term for the programmers involved.



Most of the time I speak of dishonesty, I am speaking of it in terms of being dishonest with oneself first, and the key dishonesty is when we do something that causes pain in our design, code, or project as a whole yet persist in saying that it is good.



"Self exuberance" is an even better term. HAY LOOKIT MEEEEEEeeee~~~! Reply ) ( Parent ) ( Thread From: paulto Date: July 19th, 2008 02:39 am (UTC) Disrespectful may be a better term (Link) Great writing! I have been having similar experience and thoughts since very long ago and I re-read this piece several times and enjoyed it.



A little comment though: I has always thought of the code obscuring its purpose as "disrespectful" rather than "dishonest". After reading this article I had a chance to think it over again and I still feel that "disrespectful" fits better: to me, dishonesty assumes intent and, IMO, people who write the code like that simply do not bother having a second thought about its ultimate purpose, which is to be read and maintained. That is, they may be disrespectful to the future readers (including themselves) and lazy or poorly trained or, at worst, unwilling to think -- but not really malevolent or deceitful. Reply ) ( Thread