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 4 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.
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.
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.
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/