We've been adding a lot of new code lately, particularly as part of B2G. But we have not been adding high-level in-source documentation along with that code.
The result is that it's becoming increasingly difficult to find one's way around our code. To be clear, my beef here isn't so much with a paucity of nitty-gritty comments in implementations about why we do something a particular way, but with the lack of higher-level comments in headers, IDL files, and JS services explaining what a class or interface is used for, how it interacts with other classes, and non-obvious particulars of the API it exposes. I don't want to pick on any specific examples too much, because I think the problem isn't specific examples but rather the general culture that it's OK to check in headers and interfaces without any comments. But lest you think I'm imaging this problem, I'll point to a few files that happen to be on my mind at the moment: * nsISettingsService is a device-settings API used in B2G. It's not at all clear to me how to correctly use this interface. Some questions I have include: What is the optional aMessage argument on set() for? Are the settings locks re-entrant? What can I set a setting to (they're jsvals)? Unfortunately there are no comments on the interface. * What's the difference between mozilla::system::TimeSetting and mozilla::dom::time::TimeManager? Even if I gave you the header files (which don't have any comments), I don't think it would be particularly clear what's going on. Again, the problem isn't these examples specifically -- the vast majority of B2G work I've seen has been un- or severely under-commented. I propose here that reviewers and super-reviewers should require that new code be appropriately commented, just as we require that new code be appropriately formatted. (We seem to be quite good at enforcing the latter, even though I hope we can agree that comments are, at a minimum, as important as formatting. Maybe we should put something about commenting in the style guide. :) "Appropriately commented" is obviously up to individual interpretation. But I think at a minimum, interfaces and classes defined in header files should be preceded by a sentence or two explaining their purpose and perhaps how they interact with other pieces of code. Additionally, I'd say that methods and arguments in an API (in a .idl, .ipdl, .h, or .js file) should be documented to an extent which would allow a programmer of average skill (for Mozilla) to correctly use the API without reading the implementation. That may mean that none of the methods on a particular API get any comments, which is totally fine by me. I really hope this isn't controversial. Again, the point isn't to go crazy with comments; they are merely a means to an end. -Justin _______________________________________________ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
