PR Rules - noambenami/in-memoriam GitHub Wiki

This page captures basic rules that we do not yet have static analysis checks for. It is intended to be a living list that is regularly updated/mutated as our skills evolve.

• Self PR Review: Before sending out a PR review, review it yourself. You'll be surprised by what you find.

• Single Responsibility: All modules and functions must have a single, clear responsibility. Functions should be no longer that ~20 lines of code. Modules should have no more than ~20 functions. Practice continuous refactoring to keep things clean.

  • Classes are Transformers: We view code as the implementation of data transformations. Nothing more, nothing less. Every class/module in a system should be responsible for the transformation of data from one domain to another, and no more. E.g. a service providing configuration information stored in local storage is a transformer from the local storage string to a javascript object. A react container component transforms state to a component tree, while a react visual component transforms props to markup. A REST endpoint transforms an HTTP request to a response object.

  • Transformation Domains are Documented Clearly: Every class and module, therefore, begins with an explanation of its purpose, along with the source and target domains. Once a class starts including more than the source and target domains, it is a sign that a refactoring is necessary.

• Layering: Code should be logically layered in vertical slices corresponding to coarse-grained transformation domains. For example, UI -> Business Logic -> Storage. Layers should only call down, never up: A call up from a lower layer couples the layers and makes the system brittle and difficult to test.

• Least Knowledge: Methods should never receive more data than they need to perform their work. This improves testability, lowers coupling, and encourages intentional object design.

• Favor functional iteration: Do not use while/do/for..in loops when you can compose chains of map, reduce, filter and other functional methods. Unlike the very general language-level constructs, these methods are composable, compact, and communicate intent more clearly. For example, a map function implies a data projection behavior, whereas a reduce implies a reduction of arity, e.g. the reduction of an array to a numerical value such as an average or count.

• DO NOT NEST: Computed values should always be explicitly stored into a variable with a meaningful name. For example: if(a && b || !c) { ... } is unacceptable. Ditto const d = A && b || !c ? 'foo' : 'bar'. In both of these cases, business rules are written in a way that is undocumented. Store the check in a variable that communicates the business intent. E.g. this is also wrong: const aAndBOrNotC = a && b || !c. Such a variable name provides no value at all to readers. Instead, derive the name from the meaning, for example: const hasWriteAccess = a && b || !c. Subsequent code then becomes: if (hasWriteAccess) { ... }. Which is enormously better.

• Do not Copy & Paste: Any repeated code sections will be scrutinized as they are strong signals of code smells and technical debt. Roughly 50-70% of copy and paste behavior results in a bug down the road. Nearly 100% results in technical debt, usually in the next development cycle.

• Denormalization is a Last Resort: Multiple copies of identical state in different structure is usually a bug farm. Be intentional about keeping application data normalized.

• Object Construction is a Distinct Responsibility: Most of us have been conditioned by the presence of constructors in classes to allow classes to construct instances of themselves. For example, a class that pulls its own configuration from a configuration file. Such anti-patterns make it hard to subsequently construct those objects in other ways and represent a violation of single responsibility. Favor the creation of lightweight factories for stateful objects.

• Act on Architectural Smells: We've found many instances of PRs where the architecture, or lack thereof, was generating multiple code smells and as a result concerns were raised at PR time. This is appropriate and should be encouraged: PRs are an important bulwark against the creation of unnecessary technical debt. It is 10x more expensive and difficult to fix code once it has shipped than during the PR.

• Have a Coherent and Consistent Concept Model: If your code does not communicate in a way that humans can understand, it should get rejected. Consider, for example, this:

function prependUrl(path) {
  const stringToPrefix = this.bar;
  const finalValue = path + this.bar;
  return finalValue;
}

This code incoherent: It uses the terms "prefix", "prepend", and "final" for the same exact thing. It also claims to prepend "Url" but takes "path" as an argument. Instead, we would want to see:

function prependUrl(url) {
  const stringToPrependTo = this.bar;
  const prependedString = url + stringToPrependTo;
  return prependedString;
}

While this example is contrived, we've seen hundreds of examples of similar issues where no thought was applied to naming things, which caused enormous confusion later when the code needed to be maintained.

• Testing

  • The Golden Rule: Hold test code to as high, or higher, a bar as production code. Test code is production code because test failure can result in blockage and test coverage issues can result in bugs in production.

  • Refactor for Testability: If a module is difficult to test, simplify and refactor it. Remember, pure components are the ideal test target.

  • Do not Copy & Paste Test Code: Most test code contains repeated patterns and it is very tempting to copy and paste code in order to create a new test that covers new scenarios. Such code will get rejected at PR time. Focus on factoring out common test patterns and favor data-driven tests that execute the same test case against different datasets. All modern testing frameworks have built-in support for such tests.

  • Testing Components: React components should be tested as black boxes. Tests should avoid touching internal component state or calling component lifecycle methods directly. Instead, components should be mounted, their environment should be carefully mocked, and assertions made about their rendered output.

    • Automation: Integration tests should never call a selector directly. All interaction with components under tests should be done through page/model objects. This ensures normalization of DOM navigation and enables efficient code coverage maintenance. Plus, it's more fun than crafting selectors all day.
    • Transformation: Integration tests should only automate objects, and not transform queried objects. Such queries should be delegated to the page/model object. For example, transforming a list of links into an array of name/url objects.

• Comments

  • The Bar: Comments are critical to the maintainability of a codebase. While most engineers are used to either not writing comments at all (E.g. https://github.com/mulesoft/edge-security-policies-ui/blob/develop/src/convertors/ListConvertors.js) or writing very sparse comments (E.g. https://github.com/mulesoft/api-manager-web/blob/master/app/admin/scripts/services/alertService.js) we hold a standard that is one step below that of serious frameworks (E.g. https://github.com/aspnet/EntityFrameworkCore/blob/master/src/EFCore/DbContext.cs.)
  • The Right Amount: Comments should be as short, but as complete as possible. They should provide context on why the decisions implicit in a body of code were made. They should also provide a high-level explanation of what code does if there is anything being done that is not obvious. It is up to the PR reviewer to decide what is not obvious and keep the comment bar very high.
  • Aggressively Linked: Comments should whenever possible, link to internal or external references. Exotic usage of a library, for example, should link to the article where the usage was found - be it on github or stackoverflow or elsewhere. API calls should link to the external API documentation portal. You don't need to go crazy with this, but we've repeatedly found that such links are incredibly useful for understanding how external systems are used, especially when they are used in subtle ways.
  • Useful for Building Context: We have all gone back to code we wrote in the past and not understood what on earth we were thinking. For code we did not write, building context can be even harder. Similar problems arise with comments, where we go back to comments we wrote in the dark past and cannot understand what we were saying. Again, it is part of the job of the PR reviewer to closely inspect comment, and ensure that a person who does not have full context on the code can read them and understand whatever information is intended to be communicated. As engineers, we are not trained in the art of documentation and communication, and so this is a muscle that will need to be developed and will require a commitment to code comment quality. The payoff is the long-term maintainability of the code.
  • Comments in Test Suites: There are few places where things can get as absurdly hairy as in test suites, where we often have to bend ourselves into pretzels in order to test things properly. The ideal solution involves code that is perfectly testable. The real world demands that we hold test code to as high a bar, from a code documentation point of view, as our production code. Again, the payoff is the ability to own, maintain, and evolve our codebase over time, rather than banging our heads against lines of code that break the second we, for example, add an asynchronous call to the code under test.

• React Components:

  • Architecture: Our core architecture involves composing visual components inside of layout components. This is critical to understand in order to keep the system clean, consistent, and maintainable and keep a high development rate:
    • Domain-specific pages such as for an Organization, User, of API begin with a layout component. Layout components are shared across the application and enable us to provide consistent pages and affordances and to consistently communicate the purpose of a page: Lists of entities (e.g. search results,) entity details, settings, dashboards, and designers are all distinct types of pages that require distinct, consistent layouts. For example, the entity details layout may present the entity name at the top and tabs for entity details sections. Pages may make back-end calls/perform redux actions in order to provide information needed by all components within them.
    • Layout components provide slots for content, these slots, which are simply props, should be filled with domain-specific text and domain-specific components such as forms. These should ideally never have styles explicitly assigned to them. Explicit style assignment at this level implies a specialization of the look and feel and is, by definition, antithetical to consistency. Apply custom styles/classes very carefully as a last resort.
    • Domain-specific components Compose shared components into usable components for a specific domain, for example a form for editing user properties, or a trid for rendering a tree of business groups. Domain-specific components should not be styled, and may make back-end calls/execute redux actions.
    • The components themselves should be shared across the application, and be organized near the shared layouts under a shared folder to separate them from the domain/entity-specific pages that specialize them. These components are the right place to have styles/classes.
    • Thus, pages render layouts, fill the layouts with components, and provide the data and callbacks that those components need to have to be useful. Styles are applied to shared layouts and shared components in the shared folder but not in the components folder, where the domain-specific work is done to compose components and layouts and wire them to the back-end.
  • Async: Never make web requests directly from a react component or a redux action. This coupling bolts the UI to the API and prevents code reuse. Implement standalone client SDKs for any remote service and inject these SDKs into actions using factories that create configured instances of each SDK.
  • Visual Components: Must always be pure. Do not perform side effects, api calls, or any other async task in a visual components. The principle is that such components should be reusable by anyone, anywhere simply via property settings. Everything else goes into the container components. For a good example of this, see the ShellUI component in anypoint-platform-ui. Note that visual components should derive from PureComponent.
  • Core Knowledge: Read and understand both the Main Concepts and the Advanced Concepts sections of https://reactjs.org/docs/getting-started.html.

• UI Development: See Platform UI Guidelines.