Moodle App Coding style
This document outlines the exceptions to the Coding style and JavaScript Coding Style which apply to the Moodle App and also includes rules for other technologies that are used in the app, like Typescript and Angular.
Unless otherwise specified, developers should follow the indications included on those documents.
Most rules are enforced with ESLint and won't be mentioned in this document, make sure to integrate a linter in your development environment.
Goals
Consistent coding style is important in any development project, and particularly when many developers are involved. A standard style helps to ensure that the code is easier to read and understand, which helps overall quality.
Abstract goals we strive for:
- simplicity
- readability
- tool friendliness
Note that much of the existing code may not follow all of these guidelines — we continue to upgrade this code when we see it.
TypeScript
Disabling ESLint rules
In some situations, it may be necessary to disable ESLint rules using inline comments. Although this is discouraged, it is allowed on certain use-cases.
Most of the time, however, this could be solved by refactoring code. So think twice before disabling a rule.
Warnings should be treated with the same severity as errors, even if they are allowed by the linter. The reasoning behind this is that warnings are useful when new rules are introduced that affect existing code, but new code should always conform to the rules or explicitly disable them.
Using async / await
Using async/await is encouraged, but it shouldn't be mixed with .then/.catch/.finally. Using both can make code difficult to understand. As a rule of thumb, there should only be one style in a given function.
async function greet() {
const response = await fetch('/profile.json');
const data = await response.json();
alert(`Hello, ${data.name}!`);
}
function greet() {
return fetch('/profile.json')
.then(response => response.json())
.then(data => {
alert(`Hello, ${data.name}!`);
});
}
async function greet() {
const response = await fetch('/profile.json');
return response.json().then(data => {
alert(`Hello, ${data.name}!`);
});
}
Async/await is syntactic sugar for Promises, so it should always be possible to avoid using .then/.catch/.finally.
To prevent making asynchronous operations difficult to spot, using await should be limited to simple statements such as one liners, assignments and if guards with a single condition.
If guards
Using if guards is encouraged to reduce indentation levels. They should handle edge cases and leave the main indentation level for normal code.
More concretely, when an if block contains a non-conditional ending statement, it should not chain other blocks.
getPrivateInfo() {
if (!this.isLoggedIn()) {
throw new Error('Please, log in!');
}
return this.privateInfo;
}
getPrivateInfo() {
if (!this.isLoggedIn()) {
throw new Error('Please, log in!');
} else {
return this.privateInfo;
}
}
Avoid abusing user-defined type guards
User-defined type guards are an advanced TypeScript feature that can be very useful for working in heavily typed applications. However, they can also be confusing for newcomers, so they should only be used when they are really necessary.
function hasSecret(user: User): user is { secret: string } {
return 'secret' in user;
}
function getSecret(user: User) {
if (!hasSecret(user)) {
throw new Error("This user doesn't have a secret");
}
return user.secret;
}
An alternative to defining type guards is to use built-in operations to perform type checks. For example, the following code would also take advantage of TypeScript inference:
interface User {
secret: string;
}
interface Post {
title: string;
}
function getSecret(object: User | Post) {
if ('secret' in object) {
throw new Error("This object doesn't have a secret");
}
return object.secret;
}
In some situations, the only solution is to use type assertions. Although this approach is simpler to understand and more straightforward, it is dangerous because we lose inference checks:
function hasSecret(user: User): boolean {
return 'secret' in user;
}
function getSecret(user: User) {
if (!hasSecret(user)) {
throw new Error("This user doesn't have a secret");
}
const userWithSecret = user as { secret: string };
return userWithSecret.secret;
}
Spread operator
The spread operator is allowed, but it's recommended to include a comment explaining what it is doing to make the code easier to understand. You can also replace it for simpler alternatives.
const numbers = [4, 5, 6];
const properties = { surname: 'Doe' };
console.log([1, 2, 3].concat(numbers));
console.log(Object.assign({ name: 'Mary' }, properties));
console.log(Math.max.apply(Math, numbers));
const numbers = [4, 5, 6];
const properties = { surname: 'Doe' };
console.log([1, 2, 3, ...numbers]); // Concatenate numbers.
// Create a new object including all properties and a new one.
console.log({ name: 'Mary', ...properties });
console.log(Math.max(...numbers)); // Find max number in array.
String interpolation
It is encouraged to use string interpolation using backticks if it makes the code more readable.
function greet(name: string) {
alert(`Hello, ${name}!`);
}
function greet(name: string) {
alert('Hello, ' + name + '!');
}
Avoid declaring variables using commas
In order to have cleaner diffs, it is not allowed to declare variables using commas. This also results in a better alignment of variable names, making the code more readable.
const foo = 'foo';
const bar = 'bar';
const foo = 'foo',
bar = 'bar';
Avoiding having too many optional arguments
In some situations, functions end up having a lot of optional arguments and this results in unreadable code and a cumbersome developer experience (having to pass multiple null or undefined values).
When these situations arise, a good approach to solve it is using an options object instead.
As a rule of thumb, when a method has more than two optional arguments that are not required to be used together, use an options object (better naming can be used for each particular scenario).
interface HelloOptions {
surname?: string;
emphasis?: string;
times?: number;
}
function sayHello(name: string, { surname, emphasis, times }: HelloOptions) {
surname = surname ?? '';
emphasis = emphasis ?? '!';
times = times ?? 1;
const fullname = `${name} ${surname}`.trim();
for (let i = 0; i < times; i++) {
console.log(`Hello ${fullname}${emphasis}`);
}
}
sayHello('World', { times: 3 });
function sayHello(
name: string,
surname: string = '',
emphasis: string = '!',
times: number = 1,
) {
const fullname = `${name} ${surname}`.trim();
for (let i = 0; i < times; i++) {
console.log(`Hello ${fullname}${emphasis}`);
}
}
sayHello('World', undefined, undefined, 3);
Using declaration files
Declaration files can be very useful in TypeScript and it is encouraged to use them when appropriate. But it's not recommended to abuse them either, here's some situations when it may be a good idea to use them:
- Declaring types for external dependencies — Libraries that don't include their own declarations and are missing from DefinitelyTyped (the packages you find under
@types/in npm). - Global declarations and extensions — Any variable you add to the window object can be declared by extending the
Windowinterface. The same idea applies for extending external dependencies. - Local declarations — Sometimes, it may be useful to create a dedicated declaration file when source files are growing too large. But this technique should not be used to substitute proper code organisation.
Using constants
In order to optimize Code Splitting, constants that are exported should be declared in a constants file:
// src/core/features/my-feature/constants.ts
export const MY_SERVICE_NAME = '...';
// src/core/features/my-feature/services/my-service.ts
import { MY_SERVICE_NAME } from '../constants';
export class MyService implements NamedService {
name = MY_SERVICE_NAME;
}
// src/core/features/my-feature/services/my-service.ts
export class MyService implements NamedService {
public static readonly MY_SERVICE_NAME = '...';
name = MyService.MY_SERVICE_NAME;
}
// src/core/features/my-feature/services/my-service.ts
export const MY_SERVICE_NAME = '...';
export class MyService implements NamedService {
name = MY_SERVICE_NAME;
}
In contrast, constants that are private or protected should be declared as static readonly class properties. Also, avoid calling them using this.CONSTANT form (given that they are static members):
export class MyService {
protected static readonly MY_CONSTANT = '...';
public someMethod(): void {
alert(MyService.MY_CONSTANT);
}
}
const MY_CONSTANT = '...';
export class MyService {
public someMethod(): void {
alert(MY_CONSTANT);
}
}
export class MyService {
protected static readonly MY_CONSTANT = '...';
public someMethod(): void {
alert(this.MY_CONSTANT);
}
}
Angular
Avoid calling methods in templates
Method calls should be avoided in template rendering, this includes structural directives such as ngIf or ngFor.
Angular templates can be rendered very often, and calling methods on every render could cause some unintended performance issues. For that reason, it is usually safer to rely on values rather than methods.
In some situations, a simple method that only returns a value would be acceptable, but it opens the door to become an issue if the method is refactored to do something more complicated. That's why it is discouraged to use methods altogether.
<div *ngIf="isAdmin">
<!-- Show admin content -->
</div>
<div *ngIf="site.isAdmin()">
<!-- Show admin content -->
</div>
Of course, this doesn't mean that you can't use any methods on a template. Not every method used on a template is called in every render.
For example, using methods in event handlers is fine:
<button (click)="login()">
Login
</button>
A warning about using getters
Other frameworks have patterns to solve this problem, for example Vue has Computed Properties and React has the useMemo hook.
However, Angular doesn't include a built-in pattern for these situations, so these properties should be managed as part of the logic for the component.
Be careful when using getters, which may give the wrong impression that a method is not being called:
get isAdmin(): boolean {
return this.site.isAdmin();
}
Even if this looks like using a property in the template, it is still calling a method in every render.
Maximise the number of attributes per line
There is a maximum line length of 140 characters for templates. Whenever that length is surpassed, the attributes should be distributed in multiple lines trying to reduce the number of total lines, instead of dedicating one line per attribute.
<ion-item
*ngFor="let course of courses" [title]="course.title"
[class.selected]="isSelected(course)" class="ion-text-wrap"
button detail="true"
(click)="selectCourse(course)">
<ion-label>
{{ course.title }}
</ion-label>
</ion-item>
<ion-item
*ngFor="let course of courses"
[title]="course.title"
[class.selected]="isSelected(course)"
class="ion-text-wrap"
button
detail="true"
(click)="selectCourse(course)">
<ion-label>
{{ course.title }}
</ion-label>
</ion-item>
If you are using VSCode, this should be done automatically on every save with the configuration that ships with the app.
Avoid default exports
Using default exports should be avoided for Angular applications because they cause issues with AOT compiler. Technically only components have this problem, but in order to avoid the mental load of thinking about this every time, we disallow it altogether.
@Component({
selector: 'my-component',
templateUrl: 'my-component.html',
})
export class MyComponent {}
@Component({
selector: 'my-component',
templateUrl: 'my-component.html',
})
export default class MyComponent {}
Declaring page modules
When creating a page component, it should be declared in the feature's lazy modules. Exceptionally, pages that are used by more than one module can create a page module; but this module should only declare components, it shouldn't include any routing functionality.
// file: core/features/feature/pages/index/index.ts
@Component({
selector: 'page-core-feature-index',
templateUrl: 'index.html',
})
export class CoreFeatureIndexPageComponent {}
// file: core/features/feature/feature-lazy.module.ts
const routes: Routes = [
{
path: 'feature',
component: CoreFeatureIndexPageComponent,
},
];
@NgModule({
imports: [
RouterModule.forChild(routes),
CoreSharedModule,
],
declarations: [
CoreFeatureIndexPageComponent,
],
})
export class CoreFeatureLazyModule {}
// file: core/features/feature/pages/index/index.page.ts
@Component({
selector: 'page-core-feature-index',
templateUrl: 'index.html',
})
export class CoreFeatureIndexPageComponent {}
// file: core/features/feature/pages/index/index.module.ts
@NgModule({
imports: [
CoreSharedModule,
],
declarations: [
CoreFeatureIndexPageComponent,
],
})
export class CoreFeatureIndexPageModule {}
// file: core/features/feature/pages/index/index.page.ts
@Component({
selector: 'page-core-feature-index',
templateUrl: 'index.html',
})
export class CoreFeatureIndexPageComponent {}
// file: core/features/feature/pages/index/index.module.ts
const routes: Routes = [
{
path: '',
component: CoreFeatureIndexPageComponent,
},
];
@NgModule({
imports: [
RouterModule.forChild(routes),
CoreSharedModule,
],
declarations: [
CoreFeatureIndexPageComponent,
],
})
export class CoreFeatureIndexPageModule {}