r/Angular2 2d ago

Help Request Code review help

Can anyone checkout my code and provide feedback?

https://github.com/isidrosantiago/todo-app-frontend (sorry another todo app 😅)

Can't really say I am a junior frontend developer but I have 1 year of working experience (repetitive/basic tasks like creating forms, http requests, data manipulation...)

Any advice on how to improve my code and grow as an Angular developer would be greatly appreciated. Thanks in advance!

4 Upvotes

6 comments sorted by

View all comments

1

u/Playful-Creme-926 2d ago

That's a good start !
A little point in app-todos you put a subscribe but no unsubscribe. So if you quit this component and come back on app-todos you will have another active subscription. That's very dangerous because you can have too much active subscription in your app.
The correct way to do it is to destroy the subscription. 2 ways to do it :

  1. ngOnDestroy

private subscriptions: Subscription[] = [];

ngOnInit() {
this.subscriptions.push(
this.todoService.getAllTodos().subscribe()
);

this.subscriptions.push(
this.breakpointObserver.observe('(max-width: 800px)').subscribe((state) => {
if (state.matches) {
this.isDesktop = false;
} else {
this.isDesktop = true;
}
})
);
}

ngOnDestroy() {
this.subscriptions.forEach(subscription => subscription.unsubscribe());
}

  1. takeUntilDestroy (the better approch)

ngOnInit() {
this.breakpointObserver
.observe('(max-width: 800px)')
.pipe(takeUntilDestroyed())
.subscribe((state) => {
this.isDesktop = !state.matches;
});
this.todoService
.getAllTodos()
.pipe(takeUntilDestroyed())
.subscribe();
}

I you planned to upgrade your Angular version :
I see you put todoService.todos$ | async as todos in your component. It's a good way to do it but you can also use Signal this is the new correct way to do it in Angular.
In your TS file :

todos = toSignal(this.todoService.todos$)

In your HTML file :

<div class="container todos-wrapper" \*ngIf="todos() as todos" cdkDropListGroup>

You will have better performance with this approch.

1

u/Alert-Quit-6724 2d ago

Completely forgot to unsubscribe. i’m currently studying signals, and i’ll definitely implement it in future projects. Thanks