A case for FIXMEs in production code

When writing code, I liberally sprinkle it with FIXME comments that have brief descriptions of what I think should be fixed. This includes blocks of code I think are hackish, fragile, ugly, or just could be improved. Often some slip through to a code review, and somebody tells me I should fix them before committing. This is all good and as it should be, and the comments have then served their purpose of flagging the parts that need fixing.

However, often I hear that the code is ”good enough” and I should remove the FIXMEs. There might be a company or project policy against committing them. ”Open a ticket if you think it needs improving”, I am told. ”FIXMEs are a code smell and unprofessional.”

Hogwash. FIXMEs are a sign of good code. It may be a good idea to open a ticket, but do not force me to drop the comments.

(Note: The actual keyword used is not the point here. I do not care if you use TODO instead of FIXME, or decide that FIXME is for something that cannot go into production while TODO can, if you really want to get embroiled in managerial decisions about converting all FIXMEs to TODOs because of time constraints.)

Banning FIXMEs hides problems from other programmers

Code is read much more often than it is written. That is why comments are valuable. Also, code is read by programmers much more often than tickets about it buried in some issue tracking system.

In most cases the author understands a piece of code better than anybody else. In a perfect world, we could plug a USB cable between the heads of the author and another programmer and transfer the author’s subjective, enlightened state of mind about the code base to the other programmer’s brain. Alas, our thoughts are prisoners in our minds, only to be expressed verbally in code comments at best. The goal of comments is to convey that state of mind as fully and accurately as possible to the programmer reading the code. If a policy hinders that goal, then it is a bad policy.

When reading code—either to get an understanding of a system, or while debugging a problem—few things are as valuable to me as the knowledge that the author sees a problem in a particular part of it, as indicated by a FIXME comment. That directs me to scrutinize that part more closely. Perhaps even more importantly, it lets me measure how close my understanding of the code is to the author’s state of mind when writing it.

Understanding code is not only about what the piece of code does; rather, it is about how it fits into the big picture, why it exists, why it was done this way and not some other way. Knowing that the author objects to some particularity in the code helps here immensely. Do I understand why the author has an objection to something here? Do I agree that the code could be improved? When I have a reason to believe I view the code the same way as its author, it is that meeting of minds that gives me confidence to work with the code, to refactor it and to mold it to fit whatever purpose is needed.

Needless to say, it ticks me off when some company policy robs me and my fellow programmers of this illumination in the name of some perceived professionalism.

Good programmers prefer code bases with FIXMEs

Given two otherwise similar code bases of 100k lines, but one with five hundred FIXMEs and one with zero, which one should I prefer? They are a code smell, right?

Am I to believe that the other 100k code base is a perfectly cut diamond, free of warts? Let me tell you, there are very few such code bases at that size. Either I am not given all information the author has, or all the code is so bad that nothing stands out as especially noteworthy. In the other code base, the presence of those FIXMEs tell me that the author is conscientious about code quality. They are, in a sense, a programmer’s apology for not meeting his own high expectations in this case. They are a testament to the programmer’s taste of code quality.

You can largely judge a programmer by his FIXMEs. If the code base is written by developers who use them liberally, you can also judge the code base by looking at those and getting a sense of what the authors consider sub-par for the code base. If the places marked with FIXMEs are still even reasonably okay-ish hacks (and nobody is trying to cheat), there should not be worse monsters lurking around the corner.

Sami Liedes

6 kommenttia

Kimmo Sundqvist sanoo:

A random idea. Is any system doing ”automatic lightweight tickets” out of FIXMEs, the ticket’s title being equal to the FIXME comment in the code? It might sound odd to allow changing the FIXME to WONTFIX in the ticket, and propagate that change to the code, but far less so if there was a way to sync ticket milestones to respective FIXMEs, like ”FIXME (for milestone_name): for n > 1, chaos will ensue.”

Jussi Ranta sanoo:

We have a practice to add data and author to todo comments so outdated (yes, there is a possibility of todo become outdated) and hastily written comments can be tracked. Some cases need a lengthy explanation, and the author may not be available to fix the actual case.

Of course, in production code there should not be todos left, but in practice this is not the case – even when code reviews are running well, different reviewers have different standards in quality or sometimes to fix a todo requires architectural work to code base. What ever are the reasons, from sole neglect to major architectural modifications to code, todos have their place in the code.

Even if there are tasks, user stories or issues for each todo in the backlog etc. the usage of todo in the code can be used to pinpoint the actual place where code can be improved.

In customer projects, each todo should be visible for customer as well (as a task, user story, bug etc), so that customer can tell the quality of the code which has been paid some good amount of money. This will improve production code quality further, which makes developers and customer more happy.

Jarkko Sakkinen sanoo:

I kind of see this completely opposite way 🙂 In my opinion they are a sign of ill working peer review process or low quality commit messages in the commit log. If you need to add such, it is a strong indicator that you should re-iterate the commit you are working on.

Liity keskusteluun