23 — Good & Bad Practices

Component Practices

✅ Do

// OnPush everywhere
@Component({
  changeDetection: ChangeDetectionStrategy.OnPush,
})

// Small, focused components (< 200 lines)
// Single responsibility — one component = one job

// Use signal inputs
name = input.required<string>();

// Unsubscribe properly
private destroyRef = inject(DestroyRef);
ngOnInit() {
  obs$.pipe(takeUntilDestroyed(this.destroyRef)).subscribe(...);
}

❌ Don't

// Don't use Default change detection in production
@Component({
  changeDetection: ChangeDetectionStrategy.Default,  // ❌ Slow
})

// Don't call methods in templates
<p>{{ getTotal() }}</p>          // ❌ Called every CD cycle
<p>{{ total() }}</p>             // ✅ Computed signal

// Don't subscribe without cleanup
ngOnInit() {
  this.service.data$.subscribe(d => this.data = d);  // ❌ Memory leak
}

// Don't use any
data: any;                       // ❌ Lose type safety
data: User[];                    // ✅ Typed

// Don't manipulate DOM directly
document.getElementById('x');    // ❌
@ViewChild('x') el!: ElementRef; // ✅

---

Service Practices

✅ Do

// Singleton with providedIn: 'root'
@Injectable({ providedIn: 'root' })
export class AuthService { }

// Type all HTTP responses
this.http.get<User[]>('/api/users');

// Use inject() function
private http = inject(HttpClient);

// Expose read-only state
private _user = signal<User | null>(null);
readonly user = this._user.asReadonly();

❌ Don't

// Don't put presentation logic in services
// Services = data + business logic only

// Don't use untyped HTTP
this.http.get('/api/users');           // ❌ Returns Observable<Object>
this.http.get<User[]>('/api/users');   // ✅ Returns Observable<User[]>

// Don't store component state in global services
// Keep local UI state (isOpen, isHovered) in the component

// Don't use localStorage directly
localStorage.setItem('token', t);      // ❌ Not testable, XSS risk
this.storageService.set('token', t);   // ✅ Abstracted, testable

---

Template Practices

✅ Do

<!-- Use new control flow (Angular 17+) -->
@if (user) {
  <h1>{{ user.name }}</h1>
}

@for (item of items; track item.id) {
  <app-item [data]="item" />
}

<!-- Use async pipe for observables -->
@if (user$ | async; as user) {
  <h1>{{ user.name }}</h1>
}

<!-- Use @defer for heavy sections -->
@defer (on viewport) {
  <app-heavy-component />
}

❌ Don't

<!-- Don't use index as track key for mutable lists -->
@for (item of items; track $index) { }     <!-- ❌ Causes unnecessary re-renders -->
@for (item of items; track item.id) { }    <!-- ✅ Stable identity -->

<!-- Don't subscribe in TypeScript when async pipe works -->
<!-- ❌ Manual subscribe + assign + unsubscribe -->
<!-- ✅ Just use | async in template -->

<!-- Don't nest ternaries -->
{{ a ? (b ? 'x' : 'y') : 'z' }}           <!-- ❌ Unreadable -->

<!-- Don't put complex logic in templates -->
<div [class]="getComplexClass()">          <!-- ❌ -->
<div [class]="computedClass()">            <!-- ✅ Signal/computed -->

---

RxJS Practices

✅ Do

// Use the right flattening operator
searchTerm$.pipe(switchMap(term => search(term)));     // Cancel previous
loginClick$.pipe(exhaustMap(() => login()));            // Ignore while running
files$.pipe(concatMap(file => upload(file)));           // Sequential

// Share HTTP results
data$ = this.http.get<Data>('/api/data').pipe(shareReplay(1));

// Handle errors
this.http.get('/api/data').pipe(
  catchError(err => {
    this.toast.error('Failed to load');
    return of([]);   // Fallback
  }),
);

❌ Don't

// Don't nest subscribes
this.service.getUser().subscribe(user => {
  this.service.getOrders(user.id).subscribe(orders => {  // ❌ Callback hell
    // ...
  });
});

// ✅ Use switchMap
this.service.getUser().pipe(
  switchMap(user => this.service.getOrders(user.id)),
);

// Don't forget to handle errors
this.http.get('/api').subscribe(data => { ... });  // ❌ Unhandled error crashes

// Don't use .toPromise() — deprecated
await obs.toPromise();              // ❌
await firstValueFrom(obs);          // ✅

---

Architecture Practices

✅ Do

✅ Lazy load every feature route
✅ core/ for singletons, shared/ for reusable, features/ for routes
✅ Smart (container) + Dumb (presentational) component pattern
✅ One feature = one folder with its own routes, services, models
✅ Barrel files (index.ts) for public API of shared modules
✅ Strict TypeScript (strict: true in tsconfig)
✅ ESLint + Prettier + Husky pre-commit hooks

❌ Don't

❌ Import between feature modules (use shared/ instead)
❌ Put everything in one module
❌ Skip lazy loading (huge initial bundle)
❌ Use NgModules for new projects (use standalone)
❌ Store business logic in components
❌ Skip writing interfaces/types for API responses
❌ Disable strict mode

---

Performance Anti-Patterns

---

Security Anti-Patterns

---

Code Review Checklist

□ OnPush change detection on all components
□ No methods called in templates (use computed/pipes)
□ All observables properly unsubscribed
□ track used in all @for loops with unique IDs
□ Lazy loading on all feature routes
□ HTTP responses are typed (no any)
□ Error handling on all HTTP calls
□ No direct DOM manipulation (use ViewChild/Renderer2)
□ Forms validated (client + server)
□ No secrets in frontend code
□ Accessibility: aria labels, keyboard navigation
□ Unit tests for services and critical components

---

Key Takeaways

• OnPush + Signals is the performance baseline — use it everywhere
• Async pipe > manual subscribe — less code, no memory leaks
• Use switchMap for search/navigation, exhaustMap for form submits
• Never call functions in templates — use computed() or pure pipes
• Features import from shared/ and core/, never from each other
• Strict TypeScript + ESLint catches most bugs before runtime
• Security: never trust the client — always validate on the server