Thursday, November 5, 2009


They actually want bad code



So I was in this huge meeting yesterday, and I got the shock of my life.

We were discussing how we're going to go about creating and marketing a new program which will be deployed on the servers of our clients. When I suggested I be the one to take charge of the program design and creation, and handpick my team of the best programmers in the company to write the code, I was shot down. The reason? They don't want the program to be written correctly. They don't want the code written by people who know what they're doing.

That had me completely flabbergasted. I needed more details. I asked what exactly was wrong with the way I did things? With creating the program properly? Our chief executive in charge of marketing dependability and quick maintenance boiled it down for me.

The problems with me writing the code are as follows:
No matter which language(s) we choose to build the program with, whether it be C++, PHP, C#, or something else, I'm going to make sure we use the classes and functions provided by the language most fit for use in our program. Every single function will be as clear and minimalistic and self contained as possible. And this is evil in terms of dependability and quick maintenance.

If for example we used C# with .NET and I found just the perfect class out of the few thousand provided to fit the job, and it turns out down the line some issue crops up, apparently, they can't complain to Microsoft. Microsoft will tell them no one uses that class, and it is probably buggy, and they'll put it on a todo list to be looked at several months down the line.

If I use any function or class in C++ or PHP outside of the most basic 10-20 ones that dime-a-dozen programmers learn right away, they won't be able to get someone outside our group of professionals to review and fix it.

Basically, they want the program written only using classes, functions, arrays, loops, and the least amount of standard library usage. Because a random programmer most likely will barely be familiar with anything contained within the standard library.

They would prefer reinventing built in functions, and also having them written incorrectly, in terms of output correctness, and running time. Since it means a programmer will never need to look in a manual to be able to understand a piece of code and fix it. Which is important apparently, as most can only figure out what is wrong with the logic directly in front of them, and then try to brute force correct output.

But it doesn't even stop at good code making good use of the language, instead of reinventing the wheel.

Quite often in our existing projects, I go to look at a bug report, and notice some function which works incorrectly, and in the process of fixing it, I condense the logic and make the code much better. Let me give an example.

This is very similar to an existing case we had. The code was as follows:

/*
This function creates a log on Sunday, Tuesday, Thursday
It takes as input an integer with a value of 1-7, 1 being Sunday.
*/
void logEveryOtherDay(int dayOfTheWeek)
{
if (dayOfTheWeek == 1)
{
logger.open();
logger.write("Sunday");
logger.dumpData();
logger.close();
}
else if (dayOfTheWeek == 3)
{
logger.open();
logger.write("-------------");
logger.write("Tuesday");
logger.dumpData();
logger.close();
}
else if (dayOfTheWeek == 5)
{
logger.open();
logger.write("-------------");
logger.write("Thursday");
logger.dumpData();
logger.close();
}
}


The problem reported was that logs from Sunday missed the ----- separator before it, and they'd want a log on Saturday too if ran then. When fixing it, the code annoyed me, and I quickly cleaned it to the following:


//This functions takes an integer and returns true if it's odd, and false if even
static bool isOdd(int i) { return(i&1); }

static const char *daysOfTheWeek[] = {
0, //Begin with nothing, as we number the days of the week 1-7
"Sunday", "Monday", "Tuesday", "Wednesday", "Thursday", "Friday", "Saturday"
}

/*
This function creates a log on Sunday, Tuesday, Thursday, Saturday
It takes as input an integer with a value of 1-7, 1 being Sunday.
*/
void logEveryOtherDay(int dayOfTheWeek)
{
if (isOdd(dayOfTheWeek)) //Logging isn't done on even days
{
logger.open();
logger.write("-------------");
logger.write(daysOfTheWeek(dayOfTheWeek));
logger.dumpData();
logger.close();
}
}


I think it should be obvious my code has much cleaner logic, and should be easy for any reasonable programmer to follow. I frequently do things like this. I even once went to look at a 2000 line function which had roughly a dozen bug reports against it, was a total mess, and ran really really slowly. Instead of hunting for the cause of each issue in it, I decided to scrap it. I created 2 helper functions, one 3 lines, the other 5, and rewrote the body of the function from 2000 lines to roughly 40. Instead of many nested ifs and several loops, we now had a single if and else which did exactly what they needed to, and called one of the two helper functions as needed where the real looping was done. The new function was profiled to run an order of a magnitude faster, and it passed all the test cases we designed, where the original failed a few. It now also contained 2 new features which were sorely lacking from the original. It was now also much easier to read it for correctness, as much less was going on in any section of the code.

But as this executive continued to tell me, what I did on these occasions is evil for an average programmer.

They can't comprehend a small amount of code doing so much. They can't understand what isOdd() does or is trying to do, unless they actually see its source. Its source of "return(i&1);" is just too confusing for them, because they don't know what "&1" means, nor can they comprehend how it can return true or false without containing an elaborate body of code. They can't just take the comment at face value that it does what it says it does. They are also frightened when they review different versions of a file to try to trace a bug when they see a ton of code just disappeared at some point, yet says it does more in the commit log.

So to sum it up, they don't want me, or programmers like me working on any code that is to be deployed on a client's server. When a client from Africa, or South America calls us up with a problem, they don't want to fly one of our good programmers down there to look at it. They want to make sure they can hire someone on site in one of those places to go and look at the problem and fix it quickly. Which apparently can't happen when there's no guarantee of being able to hirer a good programmer there on short notice, and other kinds of programmers can't deal with good code or standard library/class usage.

This mentality makes me very scared, although I guess it does explain to some extent why I find the innards of many rather large open source projects which are used commercially to be filled with tons of spaghetti logic, and written in a manner which suggests the author didn't really know what they were doing, nor should they be allowed to write code.

Anyone experience anything similar? Comments?

14 comments:

moldor said...

This is surprisingly similar to what my last employer, may they rot in hell, said to me.

We had an SQL2000 (yes !!) system to manage credit card authentication data, and it outputted a file daily that was about 100 characters. They would then take that file, run it through a VBscript routine that shortened the filename to the current date and time, and saved it for further processing by ANOTHER script that ran as a Windows Server "cron" job.

The filename conversion script was something like 50 lines of the most inefficient VB code I've ever seen, and I am NOT a VB programmer.

I hacked their SQL and added ONE SINGLE LINE that created the filename correctly for post-processing - the guy in charge of the system liked it and agreed - but the powers-that-were decided that the older way was best !! IDIOTS !!

Mind you, they had NO backup of this script, and NO BACKUP of the SQL statements that created the file - guess who they called 3 months after I was retrenched to get me to re-create it for them ? That was in May - they're still waiting.

Bz said...

wow... its really a shame this is the way it is - As a non-programmer who occasionally hacks at other peoples code, I actually do take it as face value that a function does exactly what it says it does. Why should I care HOW it does it?

Ethan said...

While the presented examples do sound like terrible management decisions, I don't think concern about radically changing existing code is necessarily a bad quality for management to have.
The reality is big changes (especially reimplementing things) is going to take a lot of time. On top of that, regressions will have to be fixed.
So it is easy to see how from a manager's would be very tempted to say "just fix the current code" to save man hours.
The flip side of this is of course that if you only choose the least time consuming solutions code quality will eventually suffer. Writing quality code often requires starting from scratch.
A bad manger will always choose the quickest solutions, just so he isn't responsible for missing deadlines. A good one will balance saving time and code quality.

Caprica said...

I really do hope, your isOdd function is overly simplified example, the name says exactly what it does, I could see that some devs may not understand how your function works.

When I read posts like this it really makes me question if I should be doing a CS degree.

insane coder said...

Hi Ethan.

>The reality is big changes (especially reimplementing things) is going to take a lot of time. On top of that, regressions will have to be fixed.

That's not necessarily true. Many things which I've reimplemented in the past were done pretty quickly. Also if you have test cases, there won't necessarily be any regressions. A fear of the unknown is a terrible thing to have.

>So it is easy to see how from a manager's would be very tempted to say "just fix the current code" to save man hours.

In the case I spoke about where I cut down 2000 lines to ~50, it only took me an hour to do so. It probably would've taken me longer to actually hunt down and fix the bugs in the old structure.

Although obviously if we're talking about a case where it's 3 months vs. 3 hours, there obviously is a reason to go with the latter even if it may hurt somewhat in the long wrong, which a manager will worry about later. But not every single case should be turned into that, you have to actually trust your programmers. If they say its roughly the same amount of time to rewrite it, and they guarantee it'll work better (and they're good on their word), then you have to let them.

That case where I mentioned where I reimplemented that 2000 line function? That was roughly a year ago, and we still haven't found a single regression in it.

Hi Caprica.
>I really do hope, your isOdd function is overly simplified example

Not really. I've run into cases like this all the time. Generally they are a bit more complex, but not always.

Dan said...

Wow... this is like a car manufacturer saying "let's not get tires from companies like Michelin or Good Year, it's too hard for less-experienced mechanics to be sure that they'll roll properly. It's a much better idea to reinvent our own. Even if they're slightly ovular, they'll work well enough. (Once we sand out the wheel base a bit and reinforce the suspension, that is.) We also don't need mirrors, just put some silver spray paint on a slab of metal and call it a day... What's that? A steering wheel? Eh, don't bother, people can just pull the control lines directly."

Kojot said...

This is so wrong on so many levels, but if I were you I'd start looking for better job, with some competent execs that will be happy to have your expertise and skills.

Steve Oldner said...

That was interesting. I work for a state government and we use SAP, so I code in ABAP. SAP's code is, well, complex in parts.

However, I code reports, interfaces and small applications that we use in additions to SAP's applications. Management usually wants better code, except for the immediate prodution fix or when we don't have enough testers to test thoughly in time.

I'm more fortunate than others, it seems.

Thanks for the article!

Gareth McCumskey said...

There really is no need to use Africa as an example. I am a South African developer who has had to correct serious misgivings in an open source application originally written by an American developer so your generalisation is pretty misleading.

As far as what you say, I do understand what you are talking about. We have actually spent time now trainging our Juniours techniques in researching functions in standard libraries as opposed to learning to figure out what a huge body of code is doing or rather using a library function that would perform the same function that a very large body of code would do.

insane coder said...

>There really is no need to use Africa as an example.

It's not an example, it is precisely what I was told.


>I am a South African developer who has had to correct serious misgivings in an open source application originally written by an American developer so your generalisation is pretty misleading.

Why do you think it is a generalization? *We* don't have our own programming team in South Africa. We would need to find someone qualified on site. Are you telling me there is a headhunting place there that churns out only the best for short term employment?

qubodup said...

I'm working on financial data analysis. A co-worker, who studied math wanted to introduce some statistical analysis procedures, but nobody understood a word he said and it was clear that he would not stay with the project longer than 1/2 a year. There was no consequence to his suggestion.

cottonvibes said...

Hey nice blog.
I would have coded that function similar to yours, however I would like to point out that there is a potential bug in your new function that the original didn't have.

That is, yours has the potential for array overflow if the input is greater than 7, while the original will just do nothing.
An assert or 'AND 7' would've been nicer to use;
or instead of:
"static bool isOdd(int i) { return(i&1); }"
have something like:
"static bool isValid(unsigned int i) { return (i&1) && (i<=7); }"

I know the code's comment explicitly says "inputs 1 to 7", but apparently you're dealing with horrible programmers, so probably best to have safety checks.

Krzysztof Kliś said...

You say " I frequently do things like this". Do you mean that you frequently rewrite code fixing bugs while introducing new ones (as cottonvibes described)?
If you write code with no single line of unit tests (which would surely reveal this flaw), than no wonder that the management is afraid of taking a risk. Maybe it's time to introduce TDD to improve the code quality, instead of bashing less experienced developers.

insane coder said...

cottonvibes:
Never use asserts, the idea is wrong.

You either want compile time checks, or an if. A maybe sometimes I want it, sometimes I don't flakiness is just a hallmark of incorrect logic.

As to the code, it's already protected via the caller (not presented here) that values are guaranteed to be 1-7, the date class used can't return anything else.

Yes, a check can be added that the date falls in the proper range, but that's redundant. Also, if such was a problem, the ideal place to fix it would be in the code that generates dates.

Krzysztof Kliś:
There is no bug. cottonvibes was speaking hypothetically if other code was wrong, which is not the case.

As to unit tests, we have tons and tons of them. However, unit tests is not relevant to the discussion here.

While all code is tested as its created, it's not always possible to automate tests that require human interaction.

Furthermore, automated tests, won't catch issues without output style unless parsing was added their too. Nor would unit tests catch future cases that don't exist yet, as the article describes.