Skip to content
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

chore: expand coding standards #2581

Merged
merged 2 commits into from
Jan 12, 2017
Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
122 changes: 113 additions & 9 deletions CODING_STANDARDS.md
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,8 @@ ES6 or TypeScript.
### General

#### 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 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
Expand Down Expand Up @@ -58,19 +59,122 @@ Keeping modules to a single responsibility makes the code easier to test, consum
ES6 modules offer a straightforward way to organize code into logical, granular units.
Ideally, individual files are 200 - 300 lines of code.

As a rule of thumb, once a file draws near 400 lines (barring abnormally long constants / comments),
start considering how to refactor into smaller pieces.

#### Less is more
Once a feature is released, it never goes away. We should avoid adding features that don't offer
high user value for price we pay both in maintenance, complexity, and payload size. When in doubt,
leave it out.

This applies especially so to providing two different APIs to accomplish the same thing. Always
prefer sticking to a _single_ API for accomplishing something.
prefer sticking to a _single_ API for accomplishing something.

### 100 column limit
All code and docs in the repo should be 100 columns or fewer. This applies to TypeScript, SCSS,
HTML, bash scripts, and markdown files.

### TypeScript

#### Provide function descriptions
For functions that are more complicated than a simple getter/setter, provide at least a brief
sentence explaining what the function does and/or _why_ it does something.
#### Typing
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should mention that, even though TypeScript infers it, we should include the method's return type for Dgeni's sake.

Avoid `any` where possible. If you find yourself using `any`, consider whether a generic may be
appropriate in your case.

For methods and properties that are part of a component's public API, all types must be explicitly
specified because our documentation tooling cannot currently infer types in places where TypeScript
can.

#### Fluent APIs
When creating a fluent or builder-pattern style API, use the `this` return type for methods:
```
class ConfigBuilder {
withName(name: string): this {
this.config.name = name;
return this;
}
}
```

#### Access modifiers
* Omit the `public` keyword as it is the default behavior.
* Use `private` when appropriate and possible, prefixing the name with an underscore.
* Use `protected` when appropriate and possible with no prefix.
* Prefix *library-internal* properties and methods with an underscore without using the `private`
keyword. This is necessary for anything that must be public (to be used by Angular), but should not
be part of the user-facing API. This typically applies to symbols used in template expressions,
`@ViewChildren` / `@ContentChildren` properties, host bindings, and `@Input` / `@Output` properties
(when using an alias).

Additionally, the `@docs-private` JsDoc annotation can be used to hide any symbol from the public
API docs.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about the protected keyword? Do we prefer to make it public instead? A small note about best practices may be beneficial.

#### JsDoc comments

All public APIs must have user-facing comments. These are extracted and shown in the documation
on [material.angular.io](https://material.angular.io).

Private and internal APIs should have JsDoc when they are not obvious. Ultimately it is the purview
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should probably mention the @docs-private directive for excluding APIs that shouldn't be user-facing.

of the code reviwer 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:
```ts
/** The label position relative to the checkbox. Defaults to 'after' */
@Input() labelPosition: 'before' | 'after' = 'after';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps, in general, we should be turning these into types? E.g. type MdLabelPosition = 'before' | 'after'; in order to allow users to annotate their code?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably, but I don't think that is general enough for the coding guidelines.

```

Methods blocks should describe what the function does and provide a description for each parameter
and the return value:
```ts
/**
* Opens a modal dialog containing the given component.
* @param component Type of the component to load into the dialog.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't want to mention the parameter type?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The type is already in the TypeScript annotations. AFAIK Dgeni picks those up properly.

* @param config Dialog configuration options.
* @returns Reference to the newly-opened dialog.
*/
open<T>(component: ComponentType<T>, config?: MdDialogConfig): MdDialogRef<T> { ... }
```

Boolean properties and return values should use "Whether..." as opposed to "True if...":
```ts
/** Whether the button is disabled. */
disabled: boolean = false;
```

#### Naming

##### General
* Prefer writing out words instead of using abbreviations.
* Prefer *exact* names over short names (within reason). E.g., `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.

##### Classes
Classes should be named based on what they're responsible for. Names should capture what the code
*does*, not how it is used:
```
/** NO: */
class RadioService { }

/** YES: */
class UniqueSelectionDispatcher { }
```

Avoid suffixing a class with "Service", as it communicates nothing about what the class does. Try to
think of the class name as a person's job title.

##### Methods
The name of a method should capture the action that is performed *by* that method.

### Angular
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we mention the forRoot replacement solution?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure that's general enough to belong in the coding guidelines.


#### Host bindings
Prefer using the `host` object in the directive configuration instead of `@HostBinding` and
`@HostListener`. We do this because TypeScript preserves the type information of methods with
decorators, and when one of the arguments for the method is a native `Event` type, this preserved
type information can lead to runtime errors in non-browser environments (e.g., server-side
pre-rendering).


### CSS

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with most of this, but the "don't use flex" part has always bothered me. Do we have an example of this weird baseline behavior that we're afraid of? It's hard to tell exactly what the issue is from reading the spec...

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One from the archives!

TL;DR The text baseline display: flex and inline-flex is computed from the first child of the flex container, which is different from display: block and inline-block. If you put two of these next to each other, the browser will align the baselines and the elements will be misaligned.

Overall I think the messaging of "be cautious" with the two specific cases is good.

Expand All @@ -83,8 +187,8 @@ elements like input and button.

#### Use lowest specificity possible
Always prioritize lower specificity over other factors. Most style definitions should consist of a
single element or css selector plus necessary state modifiers. Avoid SCSS nesting for the sake of
code organization. This will allow users to much more easily override styles.
single element or css selector plus necessary state modifiers. **Avoid SCSS nesting for the sake of
code organization.** This will allow users to much more easily override styles.

For example, rather than doing this:
```scss
Expand Down Expand Up @@ -123,7 +227,7 @@ The end-user of a component should be the one to decide how much margin a compon
This makes it easier to override styles when necessary. For example, rather than

```scss
:host {
the-host-element {
// ...

.some-child-element {
Expand All @@ -134,7 +238,7 @@ This makes it easier to override styles when necessary. For example, rather than

you can write
```scss
:host {
the-host-element {
// ...
color: red;
}
Expand Down