Add guidelines on how to write effective comments

[NicolasMassart]

Commenting Code: Why, Not How

1. Consider different kind of comments: code users comments (doc) vs code implementation comments (clarification comment).

   • Code user comments are usually API or contract documentation for users who will use your library. Written in the code in some format like JS/TSDoc or OpenAPI. It explains how to use the code, parameters, setup, config, …
   • Code implementation doc is for other devs who will have to understand and change your implementation. It has to explain why you implemented it this way compared to other options, what traps you worked around, what bug it fixes, what optimisation it provides.

2. Code Implementation Doc Comments Why, Not How: Code Shows How

   • We’ve all heard “code comments itself”
   • Example: Instead of writing // convert to hex value, which is obvious from the code addHexPrefix(toHexadecimal(props.chainId)), write // the chainId hex value is required for JSON-RPC call, see EIP-695.
   • Code Implementation Doc also provide some context and history when we can’t rely on other sources: people leave, Slack is purged, repos are forked, centralised tools can shutdown.
   • Focus in the reasons why the code is done that way, what you’ve already tried and what is not a possible replacement.
   • Reference: Code Comments: Good Practices

3. Do Not Comment the Obvious

   • Example: Avoid comments like // setting x to 5 for a line of code like x = 5.

4. Keep it Simple and Short

   • Example: Instead of a long comment explaining a complex algorithm, break it down into smaller parts and comment on those.
   • Corollary: If commenting requires too much comment, it probably means your code should be split.

5. If Review Requires In depth Explanation

   • Example: If a change in code involves a change in the architecture, you can for instance include a diagram in the PR description.
   • Github allows Mermaid code in your README and PR comments or descriptions.
   • Do not hesitate to explain your code to someone and see what questions they have. If you keep your code unchanged, add the questions and answers in the code as comments like a small FAQ.
   • Reference: https://github.blog/2022-02-14-include-diagrams-markdown-files-mermaid/

6. Provide Examples of How to Use Your Code
   

7. Code Comment vs PR Comment vs Commit Messages: When and Why?

   • Code comments are for explaining why and what the code is doing.

   /**
      * create a new delete regulation for the user.
      * This is necessary to respect the GDPR and CCPA regulations.
      * Check Segment documentation for more information.
      * https://segment.com/docs/privacy/user-deletion-and-suppression/
      * @returns Promise containing the status of the request.
      * Await this method to ensure the request is completed.
      */

   • PR comments are for explaining the context and reason for the change and ease review.

     NOTE: We discussed splitting this PR to extract the SDK patch in its own PR. But I would like to first try in only one PR, first because it's not so big, just 11 files, we've seen worse, but mostly because looking at the patch without the context of the MetaMetrics class seems strange and more complicated to me and could lead to mistakes or questions that are answered by the Metametrics code itself.However, if you believe I should absolutely make it in a separate PR, I will do it. Thanks.

   • Commit comments are to explain the global commit batch changes and provide pointers for more info about the timeline of the changes. Example of questions a dev could have when looking at your commit: “Why did they do this change twice in a two days interval? How they had to revert between both commits because of bug on other class XYZ as indicated on commit…”

   • Reference: Pull Request Reviews Good Practices, How to Write a Git Commit Message

8. Write it for Non-Native English Readers

   • Example: Avoid idioms, slang, and complex vocabulary. Keep sentences short and clear.
   • Reference: Google doc guidelines, Microsoft doc guidelines

9. it Works for Humans, it will work even better for AI and search engines

   
   • Do not hesitate to feed ChatGPT with your code and ask it for review:

   ChatGPT

   The code is well-structured and follows good practices, but there are a few areas where it could be improved:

   1. Error Handling: While there is some error handling in place, it could be more comprehensive. For example, in the #createSegmentDeleteRegulation method, it only logs the error and returns a status. It might be more useful to throw the error so that the calling code can decide how to handle it.
   2. Environment Variables: The code uses environment variables directly. It would be better to validate these variables at the start of the application. If any are missing or invalid, the application should report an error and stop running. This prevents runtime errors caused by missing or invalid configuration.
   3. Type Safety: The code uses TypeScript, but there are a few places where it uses any type (e.g., const { data } = response as any;). Using any bypasses type checking and can lead to runtime errors. It would be better to define interfaces for these data structures to ensure type safety.
   4. Code Duplication: There are some duplicated code blocks (e.g., this.#trackEvent(event, { anonymous: true, ...properties });). This could be refactored into a separate method to follow the DRY (Don't Repeat Yourself) principle.
   5. Async/Await Usage: In some methods like group, addTraitsToUser, the code returns Promise.resolve() when this.enabled is false. Instead, these methods could be declared as regular (non-async) methods that return immediately when this.enabled is false, and return a promise only when necessary.
   6. Singleton Pattern: The singleton pattern is used here, which is fine for this use case. However, it's worth noting that this pattern can make testing more difficult and can lead to problems if the class ever needs to be extended. It may be worth considering other patterns, such as dependency injection, which can offer more flexibility.

   • If you believe a sentence is to complex or can be misleading, translate it with Google Translate to a language you can’t read and then translate it back to english. If it still makes sense, use it.

10. Do not count on external tools

    Sadly all conversations in Slack get deleted after some months and I’m also unsure if [colleague name] will remember. Maybe a strategy is to comment it out and see if during QA there’s an issue.

    • This emphasises the importance of good commenting practices for future reference and understanding.
    • Let's say you and your colleague had a discussion on Slack about a certain piece of code that handles user authentication. You both agreed on a unique approach to handle a specific edge case. This approach might not be immediately obvious to anyone reading the code.
      Instead of relying on the Slack conversation (which will eventually be deleted) or your colleague's memory, you decide to comment the code like this:

    // Unique approach to handle edge case where user session might expire while
    // they are in the middle of a critical task.
    // This was discussed with [colleague name] on [date].
    // We decided to refresh the session in the background to ensure the user
    // can complete their task without interruption.
    
    refresh_user_session(user)

    This comment provides context for the decision, mentions the discussion, and explains the reasoning behind the code. This way, anyone who reads the code in the future will understand why it was written this way.

[MajorLift]

Thanks for addressing this important topic! Some initial observations:

• The section on PR comments vs. commit comments doesn't seem relevant to our workflow, as we squash our PRs. There's also some overlap with the contents of the pull-requests article:
  • See https://github.com/MetaMask/contributor-docs/blob/main/docs/pull-requests.md#clean-up-the-commit-message
  • See https://github.com/MetaMask/contributor-docs/blob/main/docs/pull-requests.md#use-the-description-to-explain-the-need-and-solution
• This article will probably need to include information on JSDoc: when we require JSDoc comments, content and style rules we enforce with eslint, how we use it with typedoc, etc.
• Maybe we could point to more authoritative sources e.g. official style guides or coding standards, publications from reputable authors, etc. rather than blog articles?
• Is this a proposal, working draft, or submission for review? If this is intended to be a contributor-docs article, could you submit it as a PR under the docs/ directory?

[NicolasMassart]

Thanks for addressing this important topic! Some initial observations:

Thanks for your feedback @MajorLift!

• The section on PR comments vs. commit comments doesn't seem relevant to our workflow, as we squash our PRs.

It depends how you do it. I usually use the github suggested merged commit messages in the squash commit. I don't want to have the PR description in the commit, it's not the place to have it as goals are very different. But I can clarify this in the changes.

There's also some overlap with the contents of the pull-requests article:

• See https://github.com/MetaMask/contributor-docs/blob/main/docs/pull-requests.md#clean-up-the-commit-message
• See https://github.com/MetaMask/contributor-docs/blob/main/docs/pull-requests.md#use-the-description-to-explain-the-need-and-solution

Yes absolutely, these changes require to check the existing doc and rework some content.

• This article will probably need to include information on JSDoc: when we require JSDoc comments, content and style rules we enforce with eslint, how we use it with typedoc, etc.

Yes, or at least links to commonly used good practices.

• Maybe we could point to more authoritative sources e.g. official style guides or coding standards, publications from reputable authors, etc. rather than blog articles?

Yes, like the already sourced Google or MS guidelines. Also the provided blog sources are more inspirations than actual sources of truth. The real source of truth will be what we write based on sources that can be sometimes considered as non official. The most important is that it fits our needs and makes sense.

• Is this a proposal, working draft, or submission for review? If this is intended to be a contributor-docs article, could you submit it as a PR under the docs/ directory?

This is an issue ticket, it raises the need for change and provides an example of the content we could include. It's a proposal and a reminder. Nothing set in stone here.

[MajorLift]

Sounds good on all fronts!

The most important is that it fits our needs and makes sense.

The only point I'd want to add is that contributor docs are intended to be external-facing, so we might want to be extra careful about which sources we endorse or overall "polish," so to speak.

[NicolasMassart]

Add info about how it can be helpful to have Gherkin formatted comments and Mermaid diagrams in PRs and code. See https://github.com/MetaMask/metamask-mobile/pull/8090/files#diff-c38099d02abf262b9f7c7bb6dd4e238c2114e6ff8515053c2097f200eee02936R28-R72 as an example