Take the 2-minute tour ×
Programmers Stack Exchange is a question and answer site for professional programmers interested in conceptual questions about software development. It's 100% free, no registration required.

My coworker who is a senior guy is blocking me on a code review because he wants me to name a method 'PerformSqlClient216147Workaround' because it's a workaround for some defect ###. Now, my method name proposal is something like PerformRightExpressionCast which tends to describe what the method actually does. His arguments go along the line of: "Well this method is used only as a workaround for this case, and nowhere else."

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

share|improve this question
    
Just a clarification: the defect ### is in an external component called SqlClient, it was filed in 2008, it most probably won't be fixed soon and it's out of our power, so this method is a part of a design to "work-around" that issue... –  Bojan Feb 13 '13 at 1:05
2  
It still read like a rant so I refocused and retitled the question to the core of what you are asking. I feel that it is a fair question now. Questions like "My superior did X, he is so wrong... RIGHT GUYS?" are typically closed as Not Constructive. –  maple_shaft Feb 13 '13 at 1:54
39  
Assume the temporary workaround will become permanent. They always do. –  user16764 Feb 13 '13 at 3:49
2  
@maple_shaft - excellent save-edit on the question. –  GlenH7 Feb 13 '13 at 18:24
2  
Bug #s are for comments and version control commit notes, not method names. Your coworker should be slapped. –  Erik Reppen Feb 15 '13 at 4:51
show 3 more comments

9 Answers

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.

share|improve this answer
5  
Having a direct http link to the bug in the documentation comment would be a good idea. You can also define your own annotations @Workaround(216147) –  Sulthan Feb 13 '13 at 9:41
2  
or @warning This is a temporary hack to... or TODO: fix for ... –  BЈовић Feb 13 '13 at 11:34
1  
@Sulthan - Sure...Lets link to the defect database that might not exist in a few years. Describe the defect, put the defect # ( and date ), document its a workaround but links to internal tools that can change seems like a bad idea. –  Ramhound Feb 13 '13 at 14:05
4  
@Ramhound You should consider your defect database and change history as being at least as valuable as the source code. They tell you the full story of why something was done, and how it got to be like this. The next person will need to know. –  Tim Williscroft Feb 15 '13 at 7:10
1  
Code (in this case, the method name) should self-document what it does. Comments should explain why the code exists or is structured a certain way. –  Aaron Kurtzhals Feb 15 '13 at 22:16
show 2 more comments

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)...

share|improve this answer
40  
+1 Nothing is more permanent than a temporary fix that works. –  Mathew Foscarini Feb 13 '13 at 2:44
2  
"is widely accepted"[citation needed] –  user4051 Feb 13 '13 at 9:18
3  
@Graham: Is this good enough, or does it need to be a peer reviewed, published paper in a reputable jorunal.... stackoverflow.com/questions/123936/… –  mattnz Feb 13 '13 at 21:06
add comment

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.

share|improve this answer
    
Ok, we're getting somewhere: Why do you think that the source code is not a good place for bug numbers? –  Bojan Feb 13 '13 at 1:14
1  
Because bug tracking systems and version controls are a better place for that. It's not exactly the same but is similar to: stackoverflow.com/q/123936/240613 –  MainMa Feb 13 '13 at 1:19
    
Ok, makes sense in general. But if you're dealing with a workaround, as in a deviation from the main design, I guess it's OK to leave a comment explaining what's been done (and possibly a defect number in the comments) so that someone who is reading code can understand why something has been done a certain way. –  Bojan Feb 13 '13 at 1:27
2  
I though only marketing people could argue the logic of adding something new that is obsolete. –  mattnz Feb 13 '13 at 3:04
1  
If why the code does what it does to work around the bug isn't obvious from reading and you need a lengthy explanation for why the workaround does what it does, including a reference to where external documentation can be found in a comment is IMO reasonable. Yes you can use your source controls blame tool to find out what change the workaround was made as part of and get the explanation there but with a large code base, and especially after refactoring elsewhere ends up fiddling with the workaround doing so can become time consuming. –  Dan Neely Feb 13 '13 at 14:09
show 6 more comments

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.

share|improve this answer
2  
I agree as it seems PerformSqlClient216147Workaround describes both exactly what the method does and it's reason for existing. I'd mark it up with an attribute (C#) specific to such things for your shop and be done with it. Numerics have their place in names... tho as above hopefully thats not only the methodology your shop uses for categorising such things. Storm in a teacup IMHO. Btw is that the real error code?? If so you're senior co worker probably has a descent chance of discovering this post which may or may not be a problem.... for you. ;) –  rism Feb 13 '13 at 6:17
add comment

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.

share|improve this answer
3  
this should be explained in the comments of the method, not its name. –  MainMa Feb 13 '13 at 1:05
2  
I agree with your answer generally, but I also agree with MainMa: meta-information about a method should be in comments, not in the name. –  Robert Harvey Feb 13 '13 at 1:11
add comment

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.

share|improve this answer
    
Point taken. But I wouldn't underestimate the power of ego :) –  Bojan Feb 13 '13 at 2:57
add comment

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!

share|improve this answer
add comment

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.

share|improve this answer
add comment

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.
share|improve this answer
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.