Angular Bad Practices: Revisited

Three years ago I published this article about bad practices often used by developers in Angular applications. If you have not read that article, it may be beneficial to read it first before this one.

Today, I want to focus on some other patterns that make our components/directives/services and other parts of our Angular apps less readable and harder to reason about. Without further ado, let’s get started!

Polluting the ngOnInit

Link to this section

ngOnInit may be the most important lifecycle hook in Angular components; it is used to initialize data, setup some listeners, create connections and so on. But sometimes this may make our lifecycle hook too overwhelming:

<>Copy
@Component({
  selector: 'some',
  template: 'template',
})
export class SomeComponent implements OnInit, OnDestroy {
  @ViewChild('btn') buttonRef: ElementRef<HTMLButtonElement>;
  form = this.formBuilder.group({
    firstName: [''],
    lastName: [''],
    age: [''],
    occupation: [''],
  })
  destroy$ = new Subject<void>();

  constructor(
    private readonly service: Service,
    private formBuilder: FormBuilder,
  ) {}

  ngOnInit() {
    this.service.getSomeData().subscribe(res => {
      // handle response
    });
    this.service.getSomeOtherData().subscribe(res => {
      // LOTS of logic may go here
    });
    this.form.get('age').valueChanges.pipe(
      map(age => +age),
      takeUntil(this.destroy$),
    ).subscribe(age => {
      if (age >= 18) {
        // do some stuff 
      } else {
        // do other stuff
      }
    });

    this.form.get('occupation').valueChanges.pipe(
      filter(occupation => ['engineer', 'doctor', 'actor'].indexOf(occupation) > -1),
      takeUntil(this.destroy$),
    ).subscribe(occupation => {
      // Do some heavy lifting here
    });

    combineLatest(
      this.form.get('firstName').valueChanges,
      this.form.get('lastName').valueChanges,
    ).pipe(
      debounceTime(300),
      map(([firstName, lastName]) => `${firstName} ${lastName}`),
      switchMap(fullName => this.service.getUser(fullName)),
      takeUntil(this.destroy$),
    ).subscribe(user => {
      // Do some stuff
    });

    fromEvent(this.buttonRef.nativeElement, 'click').pipe(
      takeUntil(this.destroy$),
    ).subscribe(event => {
      // handle event
    })
  }

  ngOnDestroy() {
    this.destroy$.next();
  }
}

Take a look at this component. It does not have very many methods — actually, it has only two lifecycles. But the ngOnInit method is, frankly speaking, terrifying. It subscribes to different form change events, fromEvent streams, it also loads lots of data. It has 40 lines of code, but we have actually omitted the contents of the subscribe callbacks; with them, it may easily be 100+ lines, which already goes against most soft guidelines. Also, we usually work with other methods and not the ngOnInit, so we will need better access to the other methods, but now we would have to scroll through this whole mess to reach them (or close/reopen ngOnInit every time we need to see it). Also, finding something inside the ngOnInit method itself becomes harder because there are so many mixed concepts and tasks.

Now take a look at this revised version of the same component:

<>Copy
@Component({
  selector: 'some',
  template: 'template',
})
export class SomeComponent implements OnInit, OnDestroy {
  @ViewChild('btn') buttonRef: ElementRef<HTMLButtonElement>;
  form = this.formBuilder.group({
    firstName: [''],
    lastName: [''],
    age: [''],
    occupation: [''],
  })
  destroy$ = new Subject<void>();

  constructor(
    private readonly service: Service,
    private formBuilder: FormBuilder,
  ) {}

  ngOnInit() {
    this.loadInitialData();
    this.setupFormListeners();
    this.setupEventListeners();
  }

  private setupFormListeners() {
    this.form.get('age').valueChanges.pipe(
      map(age => +age),
      takeUntil(this.destroy$),
    ).subscribe(age => {
      if (age >= 18) {
        // do some stuff 
      } else {
        // do other stuff
      }
    });

    this.form.get('occupation').valueChanges.pipe(
      filter(occupation => ['engineer', 'doctor', 'actor'].indexOf(occupation) > -1),
      takeUntil(this.destroy$),
    ).subscribe(occupation => {
      // Do some heavy lifting here
    });

    combineLatest(
      this.form.get('firstName').valueChanges,
      this.form.get('lastName').valueChanges,
    ).pipe(
      debounceTime(300),
      map(([firstName, lastName]) => `${firstName} ${lastName}`),
      switchMap(fullName => this.service.getUser(fullName)),
      takeUntil(this.destroy$),
    ).subscribe(user => {
      // Do some stuff
    });
  }

  private loadInitialData() {
    this.service.getSomeData().subscribe(res => {
      // handle response
    });
    this.service.getSomeOtherData().subscribe(res => {
      // LOTS of logic may go here
    });
  }
  
  private setupEventListeners() {
    fromEvent(this.buttonRef.nativeElement, 'click').pipe(
      takeUntil(this.destroy$),
    ).subscribe(event => {
      // handle event
    })
  }

  ngOnDestroy() {
    this.destroy$.next();
  }
}

The logic of the component is the same, but how we arrange our code is different. Now the ngOnInit method calls for three different methods to load the initial data from services, setup form change listeners and setup DOM event listeners (if needed). After this change, reading the component from scratch becomes easier (read the ngOnInit — understand what it gets started at a glance, and — if you need implementation details — visit the corresponding methods). Finding the source of the bugs is also relatively easier: if form listeners don’t work correctly — go to setupFormListeners and so on.

Don’t pollute your ngOnInit method — split it into parts!

Writing useless directive selectors

Link to this section

Angular directives are a powerful tool which allows us to apply custom logic to different HTML elements. In doing so, we utilize css selectors, which actually gives us way more power than we care to realize. Here is an example: imagine a directive that checks if the corresponding element’s formControl has errors and applies some style on it — let’s call it an ErrorHighlightDirective. Now let’s say we give it an attribute selector, say [errorHighlight]. It works fine, but we now have to find all form elements with formControl attribute and put our [errorHighlight] on them, which is a tedious task. But we can, of course use the [formControl] directive attribute selector itself, so out directive will look like this:

<>Copy
@Directive({
  selector: '[formControl],[formControlName]'
})
export class ErrorHighlightDirective {
 // implementation
}

Now our directive will automatically bind to all form controls in our module.

But the usage does not end there. Imagine we want to apply a shaky animation to all formControls that have a has-error class on them. We can easily write a directive and bind it using a class selector: .has-error.

Use better selectors for your directives to avoid cluttering your HTML with unnecessary attributes

Logic inside a service constructor

Link to this section

Services are classes, and as such, have a constructor, which is usually used to inject dependencies. But sometimes developers write some code/initialization logic inside it too. And sometimes this is not the best idea, and here’s why.

Imagine a service that creates and maintains a socket connection, sends data to the server in real time and dispatches even from the server. Here is a naive implementation:

<>Copy
@Injectable()
class SocketService {
  private connection: SocketConnection;

  constructor() {
    this.connection = openWebSocket(); // implementation details omitted
  }

  subscribe(eventName: string, cb: (event: SocketEvent) => any) {
    this.connection.on(eventName, cb);
  }

  send<T extends any>(event: string, payload: T) {
    this.connection.send(event, payload);
  }
}

This basic service creates a socket connection and handles interactions with it. Notice anything off?

The problem is, any time a new instance of this service is created, a new connection will open. And this just may not be the case we want!

Actually, lots of times an app will use a single socket connection, so when we use this service inside lazy loaded modules, we will get a new open connection. To avoid this, we need to remove the initialization logic from this constructor, and find another way to share the connection between lazy loaded modules. Also, we might want to have a method that allows us to reload the connection at will (essentially reopen it, for example if it unexpectedly closes):

<>Copy
@Injectable()
class SocketService {
  

  constructor(
    private connection: SocketConnection 
    // the SocketConnection itself is provided in the root of the App and is the same everywhere
  ) {  }

  // handle to reload a socket, naive implementation
  openConnection() {
    this.connection = openWebSocket();
  }

  subscribe(eventName: string, cb: (event: SocketEvent) => any) {
    this.connection.on(eventName, cb);
  }

  send<T extends any>(event: string, payload: T) {
    this.connection.send(event, payload);
  }
}

Adding new state when you can derive it from the existing state

Link to this section

Every component has its state — a set of properties that hold data essential to render the UI. State is the most important logical part of our app, so handling it correctly has great benefits.

The state can be described as original and derived. Original state can be described as independent data that exists on itself — for example, the state of being signed in. Derived state is entirely dependent on some piece of original state — for example, a text notice that says ‘Please sign in’ if the user is logged out or ‘Sign out’ if the user is signed in. Essentially, we don’t need to hold that text value anywhere — anytime we need it, we can calculate it based on the authentication state. So this piece of code:

<>Copy
@Component({
  selector: 'some',
  template: '<button>{{ text }}</button>',
})
export class SomeComponent {
  isAuth = false;
  text = 'Sign Out';

  constructor(
    private authService: AuthService,
  ) {}

  ngOnInit() {
    this.authService.authChange.subscribe(auth => {
      this.isAuth = auth;
      this.text = this.isAuth ? 'Sign Out' : 'Sign In';
    });
  }
}

will turn into this:

<>Copy
@Component({
  selector: 'some',
  template: `<button>{{ isAuth ? 'Sign Out' : 'Sign In' }}</button>`,
})
export class SomeComponent {
  isAuth = false;

  constructor(
    private authService: AuthService,
  ) {}

  ngOnInit() {
    this.authService.authChange.subscribe(auth => this.isAuth = auth);
  }
}

As you can see, the text property was derived state and was completely unnecessary. Removing it made out code easier to read and reason about.

Don’t create separate variables and properties to store derived state; calculate it whenever necessary instead

This one may seem a bit easy to spot, but when dealing with increasingly complex data, even the most experienced developers sometimes make this mistake, especially with RxJS streams. In this article I explore how this concept should be handled in RxJS Angular apps.

Conclusion

Link to this section

There are lots of mistakes one can do when writing an application with Angular. But some mistakes are very common and become patterns, that are reused and abused. Learning about the most common ones and how to avoid them can be very beneficial for our Angular apps.