4

For code, we know approaches like DRY and we tend to extract common functionality. What approaches are recommended for comments? Perhaps it's a really open question, so I'm going to go with my specific scenario:

In a project I work for, we use a library for charts, which has a few bugs. It's not very well maintained and it's difficult to change that in the near future.

I have like 8/9 files with different implementations of different charts which use this library, and today I found my self having to implement a complex workaround for each of them due to a bug in the library. Because of that, I thought of a four-or-five-line comment clarifying why is that workaround required (like the scenario that caused it and why that fix is correct)

Even though the root cause of the bug is the same, the solutions are different for each file (for each chart), so I cannot extract the solution into a common code. So I found myself having to copy the exact same comment into every file, which increases the chances of, at some point, becoming stale or out of date at some point in the future.

What approaches do you consider for repeated comments across different files? Perhaps for this particular scenario, it might make sense to create a small markdown file with the comment and link it instead of having the same comment in 7/8 locations, but I wonder if it's worth the effort when it is just 3 or 4 files. Or maybe there are other approaches for comment management.

3

Ticket and work tracking systems change. Adding defects to the backlog is not ideal because nobody is going to fix them, clearly.

Duplicating comments is not ideal either for the very reasons you listed.

Commit messages are not that great either, because there are so darn many of them. A single voice gets drowned out by the crowd.

Pick one of the files for this chart library to put the Big Explanation. In files specific to a chart that suffers from the defect, add a smaller comment explaining the conditions that trigger the defect, and refer curious readers to the chart library file containing the Big Explanation.

This allows you to record implementation specific information in comments at the location of the defect. It also gives you one place in code that divulges all the gory details.

Don't create defect tickets for things you have no intention of, and derive no benefit from fixing. Those work items will hang around long after the memory of why they exist has faded. Code sticks around longer. Keep it in code.

2
  • I like the honesty of not creating a ticket that we'll probably not care about in the middle term. The library has a mayor update in beta, which basically is a migration of their functionality, but I'm not sure if it's just that (A rewrite in typescript) or if it actually fixes stuff that were broken in the previous version (which I'm using now)
    – Gonzalo.-
    Aug 28 '20 at 20:45
  • 1
    I went for this approach, but instead of picking one of the files, I place it in a markdown file in the same folder (all the charts' implementations are in the same folder) - so all of them point to the same markdown. Because of that, I also added a bunch of github issues from the libraries to explain further the goal behind the workaround
    – Gonzalo.-
    Aug 28 '20 at 20:46
7

In this particular case, it would make sense to create a ticket in an issue tracker where the bug and the general workaround strategy are documented. Often it is possible to attach further documents to a ticket, such as stack traces or screendumps. In each affected source file, have a comment linking to the ticket and sufficient explanation how the workaround works in that file.

The advantage is that you keep a list of known bugs in one place (even if you're currently unable to fix them) and you can look up affected code files by searching for links to that ticket.

If you get around to fixing the bug (or if the library supplier delivers a fixed version) that lookup may be useful to find the code where now unnecessary workarounds can be removed.

4
  • I like your suggestion about creating a ticket to tackle this in the future.
    – Gonzalo.-
    Aug 27 '20 at 19:25
  • 1
    It would also be useful to commit all the fixes together and link the issue in the commit message, so that all the relevant changes are easy to track down when the bug is fixed
    – bracco23
    Aug 28 '20 at 1:20
  • 1
    Or even better, create a pull request to fix the bug. drewdevault.com/2020/08/17/Engineers-solve-problems.html
    – Vishnu Ks
    Aug 28 '20 at 8:27
  • While I like this answer and I agree with most of that you say, for my particular scenario I want to be honest and say that we'll probably not tackle that issue ever :) So the accepted answer fits better. I still value your answer (and I upvoted it); too bad I can't choose a second "good in general but perhaps not for my particular scenario" answer
    – Gonzalo.-
    Aug 28 '20 at 20:48
2

Would it make sense to reference the one comment from the other files.

"For addition information on this bug fix see My.Project.Namespace.Class"

Or if the namespace and class names may change then reference a work item, wiki page, or ticket, or mark up file.

Keep it Simple, as well as DRY.

1
  • yeah, I went for this approach, but instead of documenting it in one of those files, I just put the comment in a markdown file, with further explanation and links to the library's issues
    – Gonzalo.-
    Aug 28 '20 at 20:43
0

I would argue that in most cases, comments are anti-patterns. The fact that one has to justify some piece of a code with an accompanying comment means that the code is not-so-readable. You also rightly pointed out that comments and code could go out of sync.

The antidote to writing comments could be readable code together with well-named unit tests for the changes you are making. Unit test names can be as verbose and as English as you like.

At the same time, I'll acknowledge that there are certain cases when you can't fight off the urge to write a comment. For eg; there could be a gotcha that you'd somehow like to justify. I'm not sure whether the scenario you described in your question fits in to this category. Nevertheless, in such cases, use your commit messages to talk about why you made that particular code change. If you're using git, make sure that the first line of the commit message is short and sweet. Place a newline and then your five-line comment could be put in. I'm not a fan of capturing this information in ticketing systems since they would be like twins separated at birth. Commit messages are perhaps the closest "meta" for the code.

3
  • Just to clarify, in this question the idea of the comment wouldn't be to clarify the details of the workaround, but to explain how the bug in the library takes place (it's a combination of two or three pieces), which is the reason why I had to implement the workaround. I don't think there's a way unit tests could replace that
    – Gonzalo.-
    Aug 27 '20 at 19:24
  • In that case, couldn't you consider using commit messages to describe why the change was done? I have bolded that suggestion in the answer.
    – aquaraga
    Aug 27 '20 at 19:30
  • the problem I see is that that approach requires the dev to think that there's something fishy and go to the commit messages / blame to find out the comment. It's not directly seen as you'd see with a comment close to the code
    – Gonzalo.-
    Aug 28 '20 at 20:41
0

Have you considered to create an interface class to the external dependency? So you have a piece of code between your internal logic and the external library.

In the interface code you can implement the workarounds (may be in additional classes).

That would solve a few issues:

  • Your chart implementations would be clean, the just generate the chart
  • Your interface class is a clear separator
  • Your fixes for the bugs are separated so they are also clear (you could comment here to explain why you did those fixes)
  • (bonus) It allows you to maybe someday move to some library with less issues.

Reason I came to this thought was both the DRY principle you mentioned but also the Single Responsibility Principle. Your chart implementations are not responsible for covering issues with the library.

2
  • In my particular case, it sounds like an overengineering to abstract the library I'm using (it's recharts, in React), mainly because it would just forward props. The workaround varies greatly per chart (and data structure), so even if I do this, I'd still have multiple places where to put the very same comment, which is the original problem I'm facing. In the ideal world, I'd probably do this refactor you mention
    – Gonzalo.-
    Aug 28 '20 at 20:42
  • Yes that might be for your case, cannot analyze that based on the info in the question. Which kind of fixes are you making per-chart? Aug 31 '20 at 6:55

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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