What are common code smells

Are "edited by" inline comments the norm in shops that use revision control?


The lead developer on our shop insists that whenever the code changes, the programmer in charge adds an inline comment stating what they did. These comments usually look like this

We use TFS for version control, and it seems to me that comments of this nature are more suitable as check-in notes than inline noise. With TFS, you can even associate the check-in with one or more errors. Some of our older, frequently changed class files have a comment-to-LOC ratio close to 1: 1. For me, these comments make it difficult to read the code and add null values.

Is this a standard practice (or at least a common practice) in other stores?




Reply:


Usually I think such comments are bad practice, and I think this type of information is related to the SCM commit protocols. In most cases, the code is just harder to read.

I do however still often something like that for certain types of edits.

Case 1 - tasks

If you're using an IDE like Eclipse, Netbeans, or Visual Studio (or have some other way to search for text in your code base), your team may be using certain "comment tags" or "task tags". In this case this can be helpful.

I added the following from time to time while reviewing the code:

or:

I use various custom task tags for this which I can see in Eclipse in the Task View as having something in the commit logs is a good thing, but not enough when you are in a review by an executive. Meeting asked why bugfix XY was completely forgotten and slipped through. For urgent matters or really questionable pieces of code, this serves as an extra reminder (but usually I'll keep the comment brief and check the commit logs as THIS is why the reminder is here so I don't clutter the code much).

Case 2 - third party libs patches

If my product needs to repackage third-party code as a source (or library), but it has been recreated from source because it had to be patched for some reason, we will document the patch in a separate document, where we will document these "caveats" list. The source code usually includes a comment similar to the following:

Case 3 - Inobvious corrections

This one is a bit more controversial and is closer to what your senior developer is asking for.

In the product I'm working on right now, we have sometimes (definitely not a common thing) a comment like:

We only do this when the bug fix isn't obvious and the code doesn't read normally. This can be the case, for example, with browser quirks or obscure CSS fixes that you only have to implement because there is a document error in a product. In general, we would link it to our internal issue repository, which then contains the detailed reason for the bug fix and references to the documentation of the error of the external product (e.g. a security recommendation for a known Internet Explorer 6 bug) something like that like).

But as I said, it's pretty rare. And thanks to the task tags, we can go through them regularly and check whether these strange fixes still make sense or can expire (for example, if we stopped support for the faulty product that caused the error).


In some cases it's better than nothing :)

I just came across a huge statistical computation class in my code base where the header comment in the form of a changelog with the usual yadda yadda was: Reviewer, Date, Bug ID.

At first I thought of scrapping, but I noticed that the bug IDs not only did not conform to the convention of our current issue tracker, but also did not match the one on the tracker that was in use before I joined the company. So I tried to read through the code and understand what the class was doing (not a statistician) and I also tried to track down these bug reports. Coincidentally, they were quite important and would have claimed the next man's life to edit the file without him knowing anything about them as they were minor precision issues and special cases based on very specific requests from the original client. Conclusion, if it hadn't been there, I wouldn't have known. If they hadn’t been there AND I had understood the class better

Sometimes it is difficult to keep track of very old requirements like these. I still ended up removing the header, but after adding a block comment before each onerous function describing why these "strange" calculations are specific requirements.

In this case, I still thought this was bad practice, but I was glad the original developer at least built it in! Would have been better to clearly comment on the code, but I think that was better than nothing.







We use this approach for files that are not under version control. We have RPG programs that run on a mainframe, and doing much more than backing them up has proven difficult.

We use the check-in instructions for versioned source code. I think that's where they belong, not where code is crowded. It's metadata, after all.






We had this practice a long time ago in a SW store where our manager insisted that he wanted to read code files on paper and keep track of their change. Needless to say, I don't remember a specific occasion when he actually looked at a printout of a source file: - /

Fortunately, my managers have since become better acquainted with what version control systems can do (I must add that we used SourceSafe in this first shop). So I don't add version metadata to the code.


It is generally not required if your SCM and IDE allow the use of the "show annotation" function (known as "show annotation" in Eclipse anyway). You can easily see which commit (s) changed a piece of code, and the commit comments should indicate who and why.

The only time I would put a comment like this in the code is if the code is particularly strange that could cause confusion in the future. I will briefly comment on it with the bug number so that he can go to the bug report and read it in detail about it.



Obviously, over time, such comments are almost guaranteed to be wrong. If you edit a line twice, are you adding two comments? Where you go? A better process is to rely on your tools.

This is a sign of that the lead developer for some reason not on the new process to track changes and to link changes to the bug tracking system. You need to fix this problem in order to resolve the conflict.

You need to understand, Why he cannot rely on it. Are they old habits? Bad experience with the SCM system? A presentation format that doesn't work for him? Maybe he doesn't even know what tools like git blame and Perforce's timeline view (which essentially shows this, although it may not show what problem caused the change).

Without understanding his reason for the request, you will not be able to convince him.


I'm working on a Windows product that's over 20 years old. We have a similar practice that has been around for a long time. I can't count the number of times this exercise saved my bacon.

I agree that some things are superfluous. In the past, developers used this method to comment on code. When I check this I am telling them to clear the code and the comment is not required.

But often you look at code that has been modified by 20 developers for about a decade but never revised. Everyone has long since forgotten all of the requirement details that were in the original code, and there is no reliable documentation.

A comment with an error number gives me the opportunity to look up the origin of the code.

The SCM system does a lot, but not everything. Treat this like any other comment and add it when a significant change is made. The developer who's been looking at your code in more than 5 years will thank you.


If you have a VCS there is no point in mentioning every change in the comments. Mentioning a specific mistake or other important note related to something on a specific line is useful. A change can contain many changed lines, all of which are under the same commit message.


Not that I can tell.

I am writing TODOs in my code as I proceed and the goal is to remove them as I review.

We use cookies and other tracking technologies to improve your browsing experience on our website, to show you personalized content and targeted ads, to analyze our website traffic, and to understand where our visitors are coming from.

By continuing, you consent to our use of cookies and other tracking technologies and affirm you're at least 16 years old or have consent from a parent or guardian.

You can read details in our Cookie policy and Privacy policy.