Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
WIP: Code comments standard #298
base: main
Are you sure you want to change the base?
WIP: Code comments standard #298
Changes from all commits
77d67ac
775a421
7d6a867
5b6c419
3a110aa
6e7ac07
b6e3432
75743e4
24f5616
67bf20e
6056707
abaa70f
b4d9cf5
9cc1322
78c5996
1e13bb7
f9fb9d7
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't agree with this.
Sometimes these standards provide some value, but I don't think that all comments should have to comply. - Some of the most useful comments are one-liners that explain something that isn't immediately obvious from reading the code.
In fact I'd suggest that it is these sorts of standards that cause some developers to write the sort of useless comments that are complained about in the section above.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@daniel-ac-martin It is important that a comment should adhere to a standard, if that standard allows one-liners that is fine, but there should be a standard agreed within the code base to ensure that all developers can interpret and write comments consistently
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If such a standard allows free-form comments like that, does it still have value? What would we be achieving by mandating this?
Does this fall into coding standards more generally? i.e. Use a linter / code formatter.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FWIW, I'm not sure that generating Swagger/OpenAPI in this way is actually a good thing. - I think it's safer to maintain your swagger specification separately from your code. That way you can test that your code meets the specification, and it is easier to detect whether you have made a change to the spec.
To put it another way, there is a risk that by generating Swagger from code (+ comments) that you change the contract without knowing, and that your subsequent tests provide a false positive.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@daniel-ac-martin maybe the wording could be clearer, but we're not saying you must follow this standard, it is simply an informative section that this might be how the project is set-up and may be of benefit
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's an irony here that this definition isn't quite right. i.e. 0! = 1 and not 0.
It's difficult when you need to come up with these synthetic examples, of course, and I'm not saying it's a problem.
But I do think we should be aware of this irony, as one argument against comments like these is that one should just read the actual code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@daniel-ac-martin I guess this plays into the idea that developers shouldn't have to read the code necessarily, especially with IDE tools that pull in comments from methods you shouldn't then need to go into the code to understand it
But yes, creating examples is more of a headache than I expected so I think there's an element of accepting they'll never be perfect. Maybe it's more hassle than it's worth and we should remove them, if they're adding more confusion than clarity?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Think I agree @deanwhitehouseHO, maybe the examples are not adding a lot, especially if they cause more confusion than working to help understand the point
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If it's about doc-block style comments, then maybe we should just have an example with fewer caveats? ;-)
Or perhaps the comment should include a link to wikipedia with the full definition? (I'm not sure how we feel about links in comments? - I find they can be useful at times.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd argue that the most important information here is the types of the input and output. (When that is not defined in the language itself.)
I also think this example is missing something in that it doesn't tell us how error conditions will be handled. (Or is it just undefined behaviour? But then should this be called out in the comment?) e.g. What happens with
calculate_factorial(-1)
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@daniel-ac-martin we've tried to avoid adding in too much information in the examples so as not to dictate the exact approach or syntax of comments - it's definitely a good shout, but i'm not sure it adds any further value other than demonstrating what you can use comments for further
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This probably isn't possible in practice.
I think I'd suggest that developers should check whether comments need to be updated/removed when they update the code.
There's also maybe a point about keeping comments minimal, but the waters get a bit muddy with the docblock-type comments. (Which are more in the realm of documentation.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@daniel-ac-martin this aligns with the idea that refactors should maintain the same functionality, this applies to commenting and testing - this is not the same as code updates though, which may alter the functionality and therefore require comment updates and test updates.
I think we have a section about comment minimisation somewhere...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@deanwhitehouseHO: Okay, so this is analogous to the idea that a good unit test should survive refactoring as well? i.e. Test the contract with the caller and not implementation details.
I think the main source of our disagreements come from an implicit assumption that code comments == doc-block style documentation. In the case of doc-block, I think that @robertdeniszczyc2 has a point with regards to treating it as documentation rather than normal comments.
As an aside, I'm not sure how much I believe in things like doc-block. I think they can be quite low value. (But then again I suppose that is what you are trying to counteract with this standard?) I think it has more value in languages that lack types (such as PHP and JS), as the documentation can start to fill the gaps left by the language. My gut instinct is that we should be making use of things like TypeScript to fill this gap, but also that we should use tests (both unit and functional) as living documentation instead of doc-block comments. That way when a change is made that breaks the contract, we know and can update the documentation (i.e. the tests).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Known deficiencies of current code should be called out in comments.
Obviously that creates a problem when those deficiencies are ameliorated, but I think that is the lesser of the two evils.
So I think this is a bit problematic as a negative example.
Although I do agree that such a comment doesn't belong in a docblock style comment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@daniel-ac-martin I suppose there are better ways to document known deficiencies such as structured code comments (i.e.
@todo
) or using external tools. The comment here isn't necessarily a deficiency but more an observation from the developer (in this example), maybe of what they've tested - again, examples are hard so any alternatives are welcomeThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I disagree with this.
We do need to maintain professionalism, especially when working in the open. (And maybe that is what this point should be changed to.)
But, comments should be opinionated. I want a comment to tell me something that I can't tell from the code alone. e.g. How much confidence did the programmer have when he/she wrote the code? Why did they make make a particular choice/design decision? Did they intend to come back to this code to clean it up later?
On the topic of 'superiority', if a piece of code has been heavily optimised (and perhaps is a bit hard to read as a result) I'd like to know that. But perhaps that feeds into your point about whether a statement is 'unwarranted'.
Perhaps there is something about being clear over what is an objective fact vs a subjective opinion? (Though as I say, I'd still want the subjective stuff, just keep things professional.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@daniel-ac-martin I think maybe the language used here isn't as clear as we hoped. Comments being opinionated in this context is about subjectivity, I don't see any benefit to one developer writing a subjective opinion within the code as this then becomes challenging if another developer has a different opinion - do you allow both to write individual comments for the same code...
I feel that what you're describing is descriptive comments, rather than what I would define as opinionated. Aligning to other points in the standard such as being valid after a refactor are more challenging if a codebase has comments such as why a choice was made. Whereas being descriptive is what we want, comments should be explanatory but I believe this is possible without being opinionated (depending on your definition).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@deanwhitehouseHO: There are a few things to tease out in this...
With regards to developers with different opinions, the situation you describe sounds like an issue with individuals in a team. I don't think this is something that we can 'standards our way out of'. The problem has to be fixed directly in that particular team. To come back to who exactly should leave a comment, I would expect that comments should be left by the developer who wrote the code. When the code is updated the comments should be updated along with it. When there is disagreement, this should be handled through the peer review process.
Your second paragraph pertains to to the difference that I mentioned between doc-block comments and ordinary comments. I agree that opinionated/subjective remarks do not belong in the documentation for an interface. (And that's what your doc-block comments really are.) On the other hand, I do think that they belong in other, more ordinary comments, as those are really documenting the implementation rather than the interface.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this point should be included.
There's probably something around saying that debugging information should be removed for production builds, but that's not really about comments.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@daniel-ac-martin this is more important for code that a user can see directly, especially JavaScript
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@deanwhitehouseHO: Ah okay, I think that's a more general point about knowing where you code is running and keeping private information private. I don't think it's specific to comments.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@daniel-ac-martin I agree it's more specific to systems that are out of the developers domain per se (i.e. in a browser), however I feel it is specific to comments as JavaScript primarily will run on a users device with the code served directly to them. Comments in the code, in this case, should be removed before we serve it to the user for a number of reasons - keeping information private is a bit broader, although comments can be encompassed under this, but it touches on a lot more than comments.
It then comes down to, as standards, do we (to an extent) duplicate content that overlaps (i.e. security and comments), or do we have it one place - if one place, how do we decide what goes where. If this fell into a security standard, would a user reading about comments know what to exclude in their comments without reading the standard (and vice versa I suppose).
One way around that is cross linking of course, if we have a standard that covers this then would you mind sharing the link as we may just want to cross link rather than repeat ourselves here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you need to be more clear about what you mean here; I somewhat doubt the premise.
Secrets (e.g. API keys) shouldn't be baked into code full-stop. (And we should employ secret scanning tools to help prevent that.) But that is a separate issue from comments.
You also mention unauthorised individuals but it's not obvious how they would gain access to a 'production deployment' in the first place. - Don't we have bigger problems if we get to that point?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@daniel-ac-martin API keys, of sorts, are often included in web-apps as a way of connecting to backend services so these are baked into the code as is the nature of the technology. This is about ensuring that we aren't giving help to anyone who wishes to understand and attempt to exploit the code in anyway - comments themselves aren't the risk, but the information they may give to potential exploiters are.
With regards to how people may access a system, that is of course a bigger problem, but that doesn't mean we let the guard down elsewhere in my opinion
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we need to be careful to the extent that we endorse 'security through obscurity' as we shouldn't rely on it as our main defence.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is somewhat covered by the Work in the open and Threat modelling principles and patterns.
We actually make reference to reducing 'security by obscurity' in the Work in the open principle, and recommend following central government guidance on open and closed source code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removing comments won't alter performance in any meaningful way. (Unless there is something very wrong with the compiler/interpreter.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@daniel-ac-martin of course, we only mention it in passing as it does alter performance very slightly - do you see any harm in stating this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How slightly?
I do see some harm, as the more content we put out the less likely anyone is to read it. So we need to focus on what matters.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Depends on a lot of factors, where we have complex systems in JS and CSS (sorry, this is more my area so i find it easier to highlight) we can have hundreds of lines of comments in a file - these bytes all add up when being served over the network. When we extrapolate that to potentially millions of requests, then it can become fairly significant when it comes to sustainability
I totally get that, we've worked hard to reduce this as much as possible so really appreciate the feedback too!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah definitely this this is more of a HS/CSS problem rather than a Java one
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this is all about client-side JavaScript, I wonder if this is really a more general point about minimising your javascript. i.e. We should cover it elsewhere.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree, but there are exceptions. So I wonder if this is a should rather than a MUST.
I suspect this is more about whether the commented code will be useful to others or just to yourself.
I'd also put in in terms of not committing commented code. i.e. More of a Git hygiene issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@daniel-ac-martin we toyed with the idea of having
SHOULD
or similar but decided to only useMUST
as a convention based on other standards we have alreadyThis is primarily a guise for keeping VCS clean yes, but as a standard on commenting rather than version control we have avoided that directly. For arguments sake they may not use a VCS or we may have a different standard altogether for those
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's safe to say that they will be using Git, and so we should handle this under Git hygiene somehow.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a more general point around managing secrets. I don't think it should be covered here except perhaps to link off to another standard / pattern / guidance.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@daniel-ac-martin can you please provide the link to the standard that we can link to please?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not certain if we have one published publicly at this time. We've got stuff written down elsewhere that we could re-use.
If we don't have it, I'd suggest removing it here and raising an issue to add a link when we do.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have a Standard around managing secrets: https://engineering.homeoffice.gov.uk/standards/managing-secrets/