5 ms·
I'm very much of the opinion that good code self documents, and excessive use of comments and documentation is a crutch for bad code. We obviously hear this qu
by matthewmacleod 13y ago
I'm very much of the opinion that good code self documents, and excessive use of comments and documentation is a crutch for bad code.
We obviously hear this quite a lot, but I think it's inaccurate - it's bad comments and documentation that are a problem, and comments in particular are often most prolific when they're bad. Comments and documentation often suffer from rot, in that they are not kept current with the code and end up being out of date in short order.
But the fact that comments and documentation can be out of date is a reason to do them better, rather than throw them away. I'm one of those people who typically codes "comment first" - writing a simplified english overview of the steps I expect to take, implementing each of them using real code, and taking a final pass to trim or expand comments as required. So I might start with:
function doit(){
// Load data from the input
// Get the correct records
// Search for the correct field
// Output the results
}
This is followed by implementation:
function doit(){
// Load data from the input
records = Data.load()
// Get the correct records
records.uniquify(function(e) { return e.group_id })
// Search for the correct field
results = records.map(function(e) { return e.timestamp })
// Output the results
print(results)
}
We've now got stupid redundant comments of the type that cause problems. Rule of thumb for me is to decide if the statement in the comment can be trivially deduced from the code immediately following it. If it can, then the comment can go. If there's some additional assumption—perhaps about side effects or properties of data which might not be immediately obvious—then the comment can be expanded to include this information. So we might end up with something like the following:
function doit(){
records = Data.load()
// Disambiguate records by looking at the group id - records will
// always be sorted by time, so we can do this to make sure we
// only get the first in a given group.
records.uniquify(function(e) { return e.group_id })
results = records.map(function(e) { return e.timestamp })
print(results)
}
Obviously not real code, but you get the point.
In general, I find that working on "self-documenting" codebases is actually rather frustrating - especially when I'm coming in on a new project. There's an absolute load of implicit knowledge about software systems that's contained within the code, and while it should be exposed and made explicit wherever possible, it's sometimes not practical - meaning that "self-documenting" code often seems anything but.
- jheriko 13y agothanks for the detailed reply, this is an example of an 'okay' comment, but its still not really needed imo. you can fix this with variable and function names: function disambiguateRecordsByGroupID() { recordsSortedByTime = Data.load() results = recordsSortedByTime.map(function(record) { return record.timestamp }) print(results) }
- yongjik 13y agoExceptThatNobodyWantsToTypeDisambiguateRecordsByGroupID20Times. (No, IDE can only help you so much. You still lose valuable screen estate for browsing source code when we have excessively long names.) Not to mention you only captured half of the parent's comment (the "why" of the function), so it should really be something like disambiguateRecordsByGroupIDBecauseRecordsAreAlwaysSortedByTime.
- jheriko 13y agoyeah, i missed a whole line of code as well :) there should probably be a reasonable limit to the length of a name... the IDEs are great, but even without them copy-paste is pretty universal and safe for identifiers.
- pseut 13y agoTo get rid of comments, you've now hard-coded the algorithm into every call to `disambiguateRecordsByGroupID`. What happens if something changes and you need to disambiguate records by something other than group ID? It seems like a short comment is a pretty small price to pay for some additional robustness.
- ryan-thompson 13y agorecords.SortedBy(:time).DisambiguateBy(:group_id)