Angular 2 Component conditionally displaying a list of elements from an array

Question

This Angular component displays a list of polls that are fetched from the server and has a slide toggle giving the authenticated user the option to display all polls or just the ones that the user has authored. I use Angular Material and Angular Flex Layout.

I'm specifically looking for feedback regarding best practices and how I handled displaying polls conditionally, but welcome all feedback. I chose to keep most of the conditional logic in the component instead of in the template but I'm not sure that is in accordance with Angular best practices.

poll-list.component.html

<div fxLayout="column" fxLayoutAlign="center" style="margin-top: 50px;">
  <md-slide-toggle *ngIf="isLoggedIn()" (change)="displayMyPolls()" fxFlexAlign="center">Show only my polls</md-slide-toggle>
  <md-nav-list fxFlexAlign="center">
    <a md-list-item *ngFor="let poll of pollsToDisplay" [routerLink]="['/poll', poll.pollId]" class="bottom-border">
      <md-icon md-list-icon>poll</md-icon>
      <h1 md-line>{{poll?.question}}</h1>
      <p md-line>{{poll?.getTotalVotes()}} votes</p>
    </a>
  </md-nav-list>
</div>

poll-list.component.ts

import { Component, OnInit } from '@angular/core';

import { PollService } from './poll.service';
import { AuthService } from '../auth/auth.service';
import { Poll } from './poll.model';

@Component({
  selector: 'app-poll-list',
  templateUrl: './poll-list.component.html',
  styleUrls: ['./poll-list.component.css']
})
export class PollListComponent implements OnInit {
  polls: Poll[] = [];
  pollsToDisplay: Poll[] = [];
  showOnlyMyPolls = false;

  constructor(private pollService: PollService, private authService: AuthService) { }

  ngOnInit() {
    this.pollService.getPolls()
      .subscribe(
                (polls: Poll[]) => {
                    this.polls = polls;
                    this.pollsToDisplay = polls;
                }
            );
  }

  displayMyPolls() {
    this.showOnlyMyPolls = !this.showOnlyMyPolls;
    if (this.showOnlyMyPolls) {
      this.pollsToDisplay = this.polls.filter((poll) => {
        return this.authService.isLoggedInUser(poll.userId);
      });
    } else {
      this.pollsToDisplay = this.polls;
    }
  }

  isLoggedIn() {
    return this.authService.isLoggedIn();
  }

}

Answer 1

• Naming: A better name for the change event handler is toggleShowOnlyMyPolls().
• TypeScript is about types: I'm trying not to forget specifying return types of the methods -- this makes it easy to inspect/scan over the signatures, and gives compile time support.
• pollsToDisplay can be relatively cheaply calculated on the fly. That will simplify code significantly at the cost of performance (which is not an issue as long as you don't deal with at least several hundred of Poll objects).

---

Propose:

export class PollListComponent implements OnInit {
    showOnlyMyPolls = false;
    polls: Poll[] = [];

    get pollsToDisplay(): Poll[] {
        if (!this.showOnlyMyPolls)
            return this.polls;

        const loggedInUserId = this.authService.getLoggedInUserId();
        return this.polls.filter(poll => poll.userId === loggedInUserId);
    };

    constructor(private pollService: PollService, private authService: AuthService) { }

    ngOnInit(): void {
        this.pollService
            .getPolls()
            .subscribe((polls: Poll[]) => this.polls = polls);
    }

    toggleShowOnlyMyPolls(): void {
        this.showOnlyMyPolls = !this.showOnlyMyPolls;
    }

    isLoggedIn(): boolean {
        return this.authService.isLoggedIn();
    }
}

How is this code simpler?

1. pollsToDisplay used to be a field that is assigned in three different places: one in subscribe(...) and two in displayMyPolls(). Each of these places could have a bug, and this code was scattered. Now it's calculated on the fly:

   • no assignments at all;
   • everything happens in one place -- if there is a bug with poll list construction, you know where to look for it, easier to unit-test, etc.;
   • and there is no unnecessary field since get pollsToDisplay() is an accessor.

2. toggleShowOnlyMyPolls() became trivial, it really does only what it is responsible for, which is mode switching.