Angular Bad Practices: Revisited

Cleaning Up Overstuffed Lifecycle Hooks

Several years back, I shared a piece on common pitfalls developers run into with Angular. If you haven't checked it out yet, it would be a useful primer before diving into this follow-up.

This time around, I'd like to shine a light on additional habits that tend to cloud the readability and maintainability of our components, directives, and services in Angular projects. Let's jump right in.

The Overloaded ngOnInit

Among all the lifecycle hooks in Angular, ngOnInit often wears the crown for importance. It's where we kick off data loading, attach listeners, establish connections, and handle various initialization tasks. But this can easily spiral out of control:

@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 that component. At first glance, it seems compact—only two lifecycle methods exist. However, the ngOnInit itself is quite intimidating. It juggles multiple form subscription events, handles fromEvent streams, and fetches a heap of data concurrently. It spans 40 lines right now, yet we've omitted what's inside the subscribe callbacks. Including those could push it well beyond 100 lines, exceeding most reasonable style guidelines. Navigating such a component is a hassle too; since we typically interact with other methods, we're forced to scroll through this dense block each time (or collapse and expand it repeatedly just to find what we need). Searching for specific logic buried inside becomes a needle-in-a-haystack exercise because so many unrelated operations are squeezed together.

Now compare that with the refactored version of the same component:

@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 functionality remains unchanged, but the organization is vastly better. The ngOnInit now delegates to three separate methods: one for pulling initial data from services, another for hooking up form listeners, and a third for DOM event listeners if they're required. This reorganization makes onboarding much smoother—just skim the ngOnInit to grasp the big picture, then dive into specific methods when you need details. Debugging becomes more straightforward as well: if form-related listeners are misbehaving, you know to examine setupFormListeners, and so forth.

Avoid cramming your ngOnInit with too many responsibilities — break it apart!

Choosing Better Directive Selectors

Directives in Angular give us remarkable flexibility to attach custom behaviors to DOM elements. By leveraging CSS selectors, we tap into far more capability than we often appreciate. Consider this: let's say we want a directive that detects when a formControl has validation errors and conditionally applies styling to that element — we'll name it ErrorHighlightDirective. If we choose an attribute selector like [errorHighlight], it functions correctly, but we'd have to manually scan every form element with a formControl attribute and sprinkle our [errorHighlight] onto each one—a chore indeed. Alternatively, we could directly target the [formControl] directive's own selector, resulting in code like this:

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

With that approach, our directive instantly applies to every form control across the module, no extra markup required.

But the possibilities extend even further. Suppose we want to attach a shake animation to all formControls bearing the has-error class. We could craft a directive and bind it via a class selector, such as .has-error.

Opt for more precise selectors with your directives so your HTML stays clean and free of redundant attributes

Keeping Service Constructors Lean

Services, being classes, come with a constructor, and typically that's where dependency injection happens. However, some developers also toss initialization logic in there, which isn't always a wise move—here's the reasoning.

Envision a service responsible for managing a socket connection, transmitting real-time data to a server while also dispatching events received from it. Here's a rough draft:

@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 initial service sets up a socket connection and manages its lifecycle. Notice any red flags?

The core issue is that every new instance of this service spawns yet another connection. That's rarely what we intend!

In practice, most applications rely on a solitary socket connection. If we inject this service into lazy-loaded modules, each one gets its own fresh connection—undesirable. To remedy this, we should strip initialization logic out of the constructor and discover alternative means to share that single connection across lazy modules. Additionally, having an explicit method to manually recreate the connection could come in handy—say, to reopen it if it unexpectedly drops:

@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);
  }
}

Deriving State Rather Than Storing It

Every component carries state: the properties holding vital data for rendering the UI. Since state forms the backbone of our app's logic, managing it thoughtfully pays dividends.

We can categorize state into original and derived. Original state is self-contained and independent—for example, whether a user is logged in. Derived state hinges entirely on other state—like a message saying "Please sign in" when logged out versus "Sign out" when logged in. Essentially, there's no need to store that text string; we can compute it on the fly depending on the auth status. So this snippet:

@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';
    });
  }
}

becomes this:

@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);
  }
}

Notice how the text property, being derived, was extraneous. Removing it streamlines the code and improves comprehension.

Skip creating extra variables to hold derived state; compute it on demand instead

This one might appear obvious in hindsight, yet with more intricate datasets, even seasoned developers occasionally lapse—especially when RxJS streams enter the picture. In another article, I delve into how this principle plays out in RxJS-powered Angular applications.

Wrapping Up

Countless errors can creep into an Angular codebase. Yet certain missteps recur frequently enough to solidify into anti-patterns that get copied and magnified. Recognizing these common traps—and knowing how to sidestep them—can make a meaningful difference for your Angular projects.