I would not name the method as your co-worker suggested. The method name should indicate what the method does. A name like PerformSqlClient216147Workaround does not indicate what it does. If anything, use comments that describe the method to mention that it is a workaround. This could look like the following:

/**
 * Cast given right-hand SQL expression.
 *
 * Note: This is a workaround for an SQL client defect (#216147).
 */
public void CastRightExpression(SqlExpression rightExpression)
{
    ...
}

I agree with MainMa that bug/defect numbers should not appear in the source code itself but rather in the source control comments as this is meta-data, but it’s not terrible if they appear in the source code comments. Bug/defect numbers should never be used in the names of methods.

Nothing is more permanent than a temporary fix that works.

Does his suggestion look good in 10 years time? It used to be common practice to comment each change with the defect it fixed. More recently (like the last 3 decades), this style commenting is widely accepted as reducing code maintainability – and that is with mere comments, not method names.

What he is proposing is compelling evidence your QC and QA systems are significantly deficient. Tracking of defects and defect fixes belongs in the defect tracking system, not the source code. Tracing of the source code changes belongs in the source control system. Cross referencing between these systems allows tracing of defects to source code…..

The source code is there for today, not yesterday, and not tomorrow (as in, you don’t type into source what to are planing to do next year)…

So it’s a temporary solution? Then use the name suggested by the reviewer, but mark the method as obsolete, so that using it would generate a warning every time somebody is compiling the code.

If it’s not, you may always tell that 216147 makes no sense in code, since the code is not linked to the bug tracking system (it’s rather the bug tracking system which is linked to the source control). The source code is not a good place for references to bug tickets and versions, and if you really need to put those references there, do it in the comments.

Note that even in comments, the bug number alone is not very valuable. Imagine the following comment:

public IEnumerable<Report> FindReportsByDateOnly(DateTime date)
{
    // The following method replaces FindReportByDate, because of the bug 8247 in the
    // reporting system.
    var dateOnly = new DateTime(date.Year, date.Month, date.Day);
    return this.FindReportByDate(dateOnly);
}

private IEnumerable<Report> FindReportsByDate(DateTime date)
{
    Contract.Requires(date.Hour == 0);
    Contract.Requires(date.Minute == 0);
    Contract.Requires(date.Second == 0);

    // TODO: Do the actual work.
}

Imagine that the code was written ten years ago, that you’ve just joined the project, and that when you asked where could you find any information about the bug 8247, your colleagues told that there was a list of bugs on the website of the reporting system software, but the website was redone five years ago, and the new list of bugs has different numbers.

Conclusion: you have no idea what this bug is about.

The same code could have been written in a slightly different way:

public IEnumerable<Report> FindReportsByDateOnly(DateTime date)
{
    // The reporting system we actually use is buggy when it comes to searching for a report
    // when the DateTime contains not only a date, but also a time.
    // For example, if looking for reports from `new DateTime(2011, 6, 9)` (June 9th, 2011)
    // gives three reports, searching for reports from `new DateTime(2011, 6, 9, 8, 32, 0)`
    // (June 9th, 2011, 8:32 AM) would always return an empty set (instead of isolating the
    // date part, or at least be kind and throw an exception).
    // See also: http://example.com/support/reporting-software/bug/8247
    var dateOnly = new DateTime(date.Year, date.Month, date.Day);
    return this.FindReportsByDate(dateOnly);
}

private IEnumerable<Report> FindReportsByDate(DateTime date)
{
    Contract.Requires(date.Hour == 0);
    Contract.Requires(date.Minute == 0);
    Contract.Requires(date.Second == 0);

    // TODO: Do the actual work.
}

Now you get a clear view of the issue. Even if it appears that the hypertext link at the end of the comment is dead five years ago, it doesn’t matter, since you can still understand why FindReportsByDate was replaced by FindReportsByDateOnly.

I find PerformSqlClient216147Workaround more informative then PerformRightExpressionCast. There is no doubt at all in the name of the function as to what it does, why it does it or how to get more information about it. It’s an explicit function that will be super easy to search in the source code.

With a bug tracking systems that number uniquely identifies the issue, and when you pull up that bug in the system it provides all the details you need. This is a very smart thing to do in the source code, and will save future developers time when trying to understand why a change was made.

If you see a lot of these function names if your source code, then the problem isn’t your naming convention. The problem is you have buggy software.

There’s value in your coworker’s suggestion; it provides traceability by associating changes to the code with the reason, documented in the bug database under that ticket number, why the change was made.

However it also suggests that the only reason that function exists is to work around the bug. That, if the problem were not an issue, you would not want the software to do that thing. Presumably you do want the software to do its thing, so the name of the function should explain what it does and why the problem domain requires that be done; not why the bug database needs it. The solution should look like part of the application, not like a band-aid.

I think that both you and he have gotten this whole thing out of proportion.

I agree with your technical argument, but at the end of the day it won’t make that much difference, especially if this is a temporary fix that can be removed from the codebase in a few days / weeks / months.

I think you should put your ego back into its box, and just let him have his own way. (If he was listening too, I’d say that you guys need to compromise. Both egos back in their boxes.)

Either way, you and he have better things to do.

Would including the bug number inside of the method name for a temporary workaround be considered bad practice?

Yes.

Ideally, your class should best map to/implement a concept in your application flow/state. The names of APIs of this class should reflect what they do to the state of the class, so that client code can easily use that class (i.e. not specify a name that literally doesn’t mean anything unless you specifically read about it).

If part of the public API of that class basically says “perform operation Y described in document/location X” then other people’s ability to understand the API will depend on:

1. them knowing what to look for in external documentation

2. them knowing where to look for the external documentation

3. them taking the time and effort and actually looking.

Then again, your method name doesn’t even mention where “location X” is for this defect.

It just assumes (for no realistic reason whatsoever) that everybody who has access to the code, also has access to the defect tracking system and that the tracking system will still be around for as long as the stable code will be around.

At the very least, if you know the defect will always be accessible in the same location and this won’t change (like a Microsoft defect number that’s been at the same URL for the last 15 years), you should provide a link to the issue in the API’s documentation.

Even so, there may be workarounds for other defects, that affect more than the API of one class. What will you do then?

Having APIs with the same defect number in multiple classes (data = controller.get227726FormattedData(); view.set227726FormattedData(data); doesn’t really tell you much and just makes the code more obscure.

You could decide that all other defects are solved by using names descriptive of the operation(data = controller.getEscapedAndFormattedData(); view.setEscapedAndFormattedData(data);), except in the case of your 216147 defect (which breaks the design principle of “least surprize” – or if you want to put it that way, it increases the measurement of “WTFs/LOC” of your code).

TL;DR: Bad practice! Go to your room!

The major goals of a programmer should be working code, and, clarity of expression.

Naming a workaround method (….Workaround). Would seem to achieve this goal. Hopefully at some stage the underlying problem will be fixed and the workaround method removed.

To me, naming a method after a bug suggests that the bug isn’t resolved or root cause isn’t identified. In other words, it suggests there is still an unknown. If you are working around a bug in a 3rd party system, then your workaround is a solution because you know the cause – they just can’t or won’t fix it.

If part of interacting with SqlClient requires you to perform a right expression cast, then your code should read “performRightExpressionCast()”. You can always comment the code to explain why.

I have spent the past 4 and a half years maintaining software. One of the things that makes code confusing to understand when jumping in is code that is written the way it is only due to history. In other words, that isn’t how it would work if it were written today. I’m not referring to quality, but to a point-of-view.

A co-worker of mine once said “When fixing a defect, make the code the way it should have been.” The way I interpret that is “Change the code to how it would look if this bug never existed.”

Consequences:

1. Usually, less code as a result.
2. Straightforward code
3. Fewer references to bugs in the source code
4. Less risk of future regression (once code change is fully verified)
5. Easier to analyze because developers don’t have to burden themselves with learning history that is no longer relevant.

Source code doesn’t need to tell me how it got to its current state. Version control can tell me the history. I need the source code to simply be in the state needed to work. That said, an occasional “// bug 12345” comment isn’t a bad idea, but it gets abused.

So when deciding whether or not to name your method ‘PerformSqlClient216147Workaround’, ask yourself these questions:

1. If I had known about bug 216147 before writing the code, how would I have handled it?
2. What is the workaround? If somebody who has never seen this code before were to look at it, would they be able to follow it? Is checking the bug tracking system necessary to know how this code works?
3. How temporary is this code? In my experience, temporary solutions usually become permanent, as @mattnz indicates.