I don’t think zero comments should be the goal, just that minimizing the need for them is often a good idea. Why can generally be included in the code. Total = SubTotal + SalesTax; Needs no real explanation.
Code smell is just another way to compare two possible approaches. If one version can include significantly fewer comments without issue then that’s good sign.
> If one version can include significantly fewer comments without issue then that’s good sign.
Which is why I don't buy what the article tries to sell, at least not fully.
In the first example, having a few initial handle-and-exit if statements to deal with edge cases, and a main body that's clean means you don't need to comment.
The approach suggested by some here that you could just have two for loops over the input, while clever, might not be immediately obvious to everyone and as such would probably need a comment explaining it.
The latter does not need special case handling, but is less obvious.
>Why can generally be included in the code. Total = SubTotal + SalesTax; Needs no real explanation.
Sure, and if all the code you ever write is implementing some trivial school assignment then your probably fine. No one with half a brain thinks your example requires a comment, but it's a bad example.
No offense, but functionally like that shows up in school assignments and billion dollar companies back ends. However, it’s easy for even that simple idea to end up being something like: Item.FinalPrice = Item.BasePrice * CalculateModifier(Item.ItemCode, Item.OrderContext); Which sacrificed understanding for elegance.
Sure, the functionality to find that SalesTax number might take tens of thousands of hours to create and cover a multitude of egde cases. Still with proper context, structure, naming conventions, etc the why’s should be clear. If your thinking “The exceptions function adjusts for tax holidays. So of course it needs to get exceptions based on location and the order date, and then it needs each items metadata etc” then that’s a great sign.
Elegant code is elegant when it encapsulates the why’s not just the how’s.
>However, it’s easy for even that simple idea to end up being something like: Item.FinalPrice = Item.BasePrice * CalculateModifier(Item.ItemCode, Item.OrderContext); Which sacrificed understanding for elegance.
Sure, and I'd call that bad code unless it exists because there are far more considerations than sales tax. Either way I don't see how that is an example of when to or not to leave a comment.
>Sure, the functionality to find that SalesTax number might take tens of thousands of hours to create and cover a multitude of egde cases. Still with proper context, structure, naming conventions, etc the why’s should be clear.
Again, that's just not true. I have a hard time imaging that you're a professional engineer with real world experience if you've never found yourself in a situation where variable names alone could not express the _why_ behind a piece of code.
>Elegant code is elegant when it encapsulates the why’s not just the how’s
Great, not always possible. For example:
// We have a longer than normal backoff period on
// timeouts here because device XYZ is a piece of junk
// and randonly stops responding for minutes at a time
or
// version 2 of the spec switched to an XML format and
// allows the header to be anywhere above the root
// element of the document (as a processing
// instruction). We cannot define a reasonable min
// header position/length. Just read the whole file.
const size_t MinHeaderLength = std::numeric_limits<size_t>::max();
or
// Workaround issue caused by .NET 4.6.1 upgrade which
// has more restrictive certificate checks for secure
// connections. This is currently affecting SignalR
// Scaleout connections to Azure Service Bus.
AppContext.SetSwitch("Switch.System.IdentityModel.DisableMultipleDNSEntriesInSANCertificate", true);
Of course you could suss out the reasoning on your own eventually, but why force people to do that? What variable naming scheme would you use to convey those reasons?
You’re missing my point of course we sometimes need comments. It’s a question of design tradeoffs. CalculateModifier could be part of a great design or a terrible one, but the need for comments based on the near meaningless name is an obvious issue that would need to be balanced by something else. Consider these projects:
Several years ago I was updating this ancient Object Pascal program. OS X had just showed up and they finally decided to do a full rewrite, but they wanted to do this is stages. Anyway, this thing was still using Apple Talk networking not TCP/IP and I was rewriting the network stack so we could replace each system individually. Surprisingly, the code was very easy to read, but was also filled with a long legacy of past issues. Comments on 68000 processor issues could safely be ignored for example. So yes lots of comments, but most of them had become useless.
More recently, I was redeveloping a .NET website that had been built by someone in love with XML. Unfortunately, the mismatch between the way the code operated and the way the system operated meant that you needed to carefully read each comment. It had slightly fewer, but far more nessisary comments which was one sign among many that it was a horrible design.
Which is why I am talking about nessisary comments. Many comments can safely sit in source control or automated tests. Their context quickly becoming outdated. However, when a systems design nessitates a great many important comments that’s a bad sign.
Many coders will argue that these comments are the type of information that should be captured by the version control system, not in the code itself.
The biggest problem with comments is they are not part of the tested integrity of the system. In other words, they are never tested for correctness.
How many times have you come across a comment that appears to have no bearing on what the code is doing? In heavily commented code, this happens all the time because a developer (not necessarily the original developer) makes a change and does not update the comment. It could be that the developer was in a hurry and sloppy, or that the change was upstream and made the comment irrelevant, or maybe the code was copied and pasted into a context that doesn't match the comment, or whatever. The point is that comments can become stale, and a stale comment is worse than no comment at all.
This has been a debate I’ve had with a friend for awhile now. I think that clear code consists of good naming, formatting, structuring, etc., and you should only need to comment high-level functionality, weird cases, or where it’s not really possible to clarify things further. They, however, commment nearly every single line/block. Their code isn’t even bad, they just do it “just in case”. To me it feels like a lot of work for not much benefit.
> I generally only see this from extremely junior devs.
He’s definitely junior in this case. He’s only in his first programming job out of college. I tried to get him to see the error of his ways but not everyone will listen to reason. :)
If he’s actually writing good code (so has sufficient skill) and is working somewhere with decent mentorship, he’ll probably get the message soon enough. Code reviews from senior engineers should tell him to cut it out every time.
That's simply not at all true. Code can be clear as day in it's purpose, but not in it's intent because it is implementing a requirement that the next reader may not be aware of. In other worda, the comment explains _why_.
What percentage of requirements should you keep in your source code?
CSS for example might assign a button to be green. Should you trace back to the specific requirement to say why it’s green or can you trust if somone changes it to blue it’s becase the requirement changed. I would generally say the second.
Ideally, the vast majority of requirements can be treated as such. Cases where I have wanted to include the comments generally relates to brittle code where some change likely has knock on effects. Ideally such code should be avoided where possible. IMO, such things are also better cought in unit tests and documented in source control providing more context.
That said, including things like design goal can be helpful to get people familiar with the system. But again IMO, specific requirements should rarely sit in comments rather than unit tests, design documents etc.
Well, I didn't mean every requirement of course. I meant the code that gets things working the right way when a cursory view of the code doesn't tell the whole story. I wouldn't question a button color without good reason to do so, but what if that single button was green when every other was blue? What if the button we're shaped like a camel, or called an API in a non-standard way?
> I wouldn't question a button color without good reason to do so
Exactly, and I don’t think a comment about an old requirement would generally do so. Someone just gave you the new requirement which presumably replaces the old one.
But, if the comment mentions 508 usability as an issue or as you say it’s shaped like a camel, then that’s going to be an issue for any version of the website you create. Basicly, comments that make it on the minimum list are suck there and don’t become relevant when comparing different possible designs.
Code smell is about comparing designs and implementations not high level requirements. If 1/2 your customers uses JAWS then you got to do what you got to do.
The need for comments means the code is not clear. That need is a code smell.