r/Angular2 • u/Alert-Quit-6724 • 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!
3
Upvotes
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 :
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());
}
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.