From 778ad3776a5748854c805f58fec60a8cc0018be5 Mon Sep 17 00:00:00 2001 From: Jeremy Elbourn Date: Tue, 23 Jun 2020 22:32:40 -0700 Subject: [PATCH] docs: create coding standards doc (#37700) Create initial document for Angular framework coding standards. This document will evolve over time. This version contains all non-controversial rules as discussed in a recent team meeting. Some text and examples were copied from angular/components. PR Close #37700 --- .pullapprove.yml | 1 + docs/CODING_STANDARDS.md | 254 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 255 insertions(+) create mode 100644 docs/CODING_STANDARDS.md diff --git a/.pullapprove.yml b/.pullapprove.yml index 5d8ca326ac..78a460640a 100644 --- a/.pullapprove.yml +++ b/.pullapprove.yml @@ -1090,6 +1090,7 @@ groups: 'dev-infra/**', 'docs/BAZEL.md', 'docs/CARETAKER.md', + 'docs/CODING_STANDARDS.md', 'docs/COMMITTER.md', 'docs/DEBUG.md', 'docs/DEBUG_COMPONENTS_REPO_IVY.md', diff --git a/docs/CODING_STANDARDS.md b/docs/CODING_STANDARDS.md new file mode 100644 index 0000000000..7d9030cd9b --- /dev/null +++ b/docs/CODING_STANDARDS.md @@ -0,0 +1,254 @@ +# Angular Framework Coding Standards + +The coding practices in this doc apply only to development on Angular itself, not applications +built _with_ Angular. (Though you can follow them too if you really want). + +## Code style + +The [Google JavaScript Style Guide](https://google.github.io/styleguide/jsguide.html) is the +basis for Angular's coding style, with additional guidance here pertaining to TypeScript. The team +uses `clang-format` to automatically format code; automatic formatting is enforced by CI. + +## Code practices + +### Write useful comments + +Comments that explain what some block of code does are nice; they can tell you something in less +time than it would take to follow through the code itself. + +Comments that explain why some block of code exists at all, or does something the way it does, +are _invaluable_. The "why" is difficult, or sometimes impossible, to track down without seeking out +the original author. When collaborators are in the same room, this hurts productivity. +When collaborators are in different timezones, this can be devastating to productivity. + +For example, this is a not-very-useful comment: +```typescript +// Set default tabindex. +if (!attributes['tabindex']) { + element.setAttribute('tabindex', '-1'); +} +``` + +While this is much more useful: +```typescript +// Unless the user specifies otherwise, the calendar should not be a tab stop. +// This prevents ngAria from overzealously adding a tabindex to anything with an ng-model. +if (!attributes['tabindex']) { + element.setAttribute('tabindex', '-1'); +} +``` + +In TypeScript code, use JsDoc-style comments for descriptions (on classes, members, etc.) and +use `//` style comments for everything else (explanations, background info, etc.). + +### API Design + +#### Boolean arguments + +Generally avoid adding boolean arguments to a method in cases where that argument means +"do something extra". In these cases, prefer breaking the behavior up into different functions. + +```typescript +// AVOID +function getTargetElement(createIfNotFound = false) { + // ... +} +``` + +```typescript +// PREFER +function getExistingTargetElement() { + // ... +} + +function createTargetElement() { + // ... +} +``` + +You can ignore this guidance when necessary for performance reasons in framework code. + +#### Optional arguments + +Use optional function arguments only when such an argument makes sense for an API or when required +for performance. Don't use optional arguments merely for convenience in implementation. + +### TypeScript + +#### Typing + +Avoid `any` where possible. If you find yourself using `any`, consider whether a generic or +`unknown` may be appropriate in your case. + +#### Getters and Setters + +Getters and setters introduce openings for side-effects, add more complexity for code readers, +and generate additional code when targeting older browsers. + +* Only use getters and setters for `@Input` properties or when otherwise required for API +compatibility. +* Avoid long or complex getters and setters. If the logic of an accessor would take more than +three lines, introduce a new method to contain the logic. +* A getter should immediately precede its corresponding setter. +* Decorators such as `@Input` should be applied to the getter and not the setter. +* Always use a `readonly` property instead of a getter (with no setter) when possible. + + ```typescript + /** YES */ + readonly active: boolean; + + /** NO */ + get active(): boolean { + // Using a getter solely to make the property read-only. + return this._active; + } + ``` + +#### Iteration + +Prefer `for` or `for of` to `Array.prototype.forEach`. The `forEach` API makes debugging harder +and may increase overhead from unnecessary function invocations (though modern browsers do usually +optimize this well). + +#### JsDoc comments + +All public APIs must have user-facing comments. These are extracted for API documentation and shown +in IDEs. + +Private and internal APIs should have JsDoc when they are not obvious. Ultimately it is the purview +of the code reviewer as to what is "obvious", but the rule of thumb is that *most* classes, +properties, and methods should have a JsDoc description. + +Properties should have a concise description of what the property means: +```typescript + /** The label position relative to the checkbox. Defaults to 'after' */ + @Input() labelPosition: 'before' | 'after' = 'after'; +``` + +Methods blocks should describe what the function does and provide a description for each parameter +and the return value: +```typescript + /** + * Opens a modal dialog containing the given component. + * @param component Type of the component to load into the dialog. + * @param config Dialog configuration options. + * @returns Reference to the newly-opened dialog. + */ + open(component: ComponentType, config?: MatDialogConfig): MatDialogRef { ... } +``` + +Boolean properties and return values should use "Whether..." as opposed to "True if...": +```ts + /** Whether the button is disabled. */ + disabled: boolean = false; +``` + +#### Try-Catch + +Only use `try-catch` blocks when dealing with legitimately unexpected errors. Don't use `try` to +avoid checking for expected error conditions such as null dereference or out-of-bound array access. + +Each `try-catch` block **must** include a comment that explains the +specific error being caught and why it cannot be prevented. + +##### Variable declarations + +Prefer `const` wherever possible, only using `let` when a value must change. Avoid `var` unless +absolutely necessary. + +##### `readonly` + +Use `readonly` members wherever possible. + +#### Naming + +##### General + +* Prefer writing out words instead of using abbreviations. +* Prefer *exact* names over short names (within reason). For example, `labelPosition` is better than +`align` because the former much more exactly communicates what the property means. +* Except for `@Input()` properties, use `is` and `has` prefixes for boolean properties / methods. + +##### Observables + +Don't suffix observables with `$`. + +##### Classes + +Name classes based on their responsibility. Names should capture what the code *does*, +not how it is used: +```typescript +/** NO: */ +class DefaultRouteReuseStrategy { } + +/** YES: */ +class NonStoringRouteReuseStrategy { } +``` + +##### Methods + +The name of a method should capture the action performed *by* that method rather than +describing when the method will be called. For example: + +```typescript +/** AVOID: does not describe what the function does. */ +handleClick() { + // ... +} + +/** PREFER: describes the action performed by the function. */ +activateRipple() { + // ... +} +``` + +##### Test classes and examples + +Give test classes and examples meaningful, descriptive names. + +```ts +/** PREFER: describes the scenario under test. */ +class FormGroupWithCheckboxAndRadios { /* ... */ } +class InputWithNgModel { /* ... */ } + + +/** AVOID: does not fully describe the scenario under test. */ +class Comp { /* ... */ } +class InputComp { /* ... */ } +``` + +#### RxJS + +When importing the `of` function to create an `Observable` from a value, alias the imported +function as `observableOf`. + +```typescript +import {of as observableOf} from 'rxjs'; +``` + +#### Testing + +##### Test names + +Use descriptive names for jasmine tests. Ideally, test names should read as a sentence, often of +the form "it should...". + +```typescript +/** PREFER: describes the scenario under test. */ +describe('Router', () => { + describe('with the default route reuse strategy', () => { + it('should not reuse routes upon location change', () => { + // ... + }); + }) +}); + +/** AVOID: does not fully describe the scenario under test. */ +describe('Router', () => { + describe('default strategy', () => { + it('should work', () => { + // ... + }); + }) +}); +```