Angular Code Review Best Practices - Refactoring From Junior Level to Senior
In this post, I'll review some Angular code and then upgrade the junior developer's code to a more advanced level.
What We Have
Here is the project we have.
It's a table displaying various issues. We render the name, message, and status, with issues being either active or resolved. We can select specific items, and the total number of selected elements is shown at the top. Additionally, we can toggle all items using the header of the page. Resolved issues cannot be selected.
Now let's examine the code.
export class AppComponent {
issues: any = [
{
id: 'c9613c41-32f0-435e-aef2-b17ce758431b',
name: 'TypeError',
message: "Cannot read properties of undefined (reading 'length')",
status: 'open',
numEvents: 105,
numUsers: 56,
value: 1,
},
{
id: '1f62d084-cc32-4c7b-943d-417c5dac896e',
name: 'TypeError',
message: 'U is not a function',
status: 'resolved',
numEvents: 45,
numUsers: 34,
value: 1,
},
{
id: 'd4febf2b-022e-45ff-a70b-cea24234f8b5',
name: 'TypeError',
message: 'can\'t define property F: "obj" is not extensible',
status: 'open',
numEvents: 31,
numUsers: 21,
value: 1,
},
{
id: 'ead13b50-3662-4150-99a3-0b1e4e8e4b5b',
name: 'TypeError',
message: 'setting getter-only property R',
status: 'open',
numEvents: 26,
numUsers: 24,
value: 1,
},
{
id: 'a0ffc9a5-7105-4640-92b2-5c360db976bf',
name: 'ReferenceError',
message: 'C is not defined',
status: 'open',
numEvents: 12,
numUsers: 11,
value: 1,
},
{
id: '01f6f953-70ad-46cf-b863-c7bfd95e5626',
name: 'SyntaxError',
message: 'missing name after . operator',
status: 'resolved',
numEvents: 15,
numUsers: 13,
value: 1,
},
];
}
Within our AppComponent, we maintain a list of issues, represented as an array of objects containing an id, name, message, status, and additional values.
The type here is any which is already bad.
<bad-table [issues]="issues"></bad-table>
Here's our HTML. We render our bad-table component here and provide the list of issues. Essentially, the inputs seem correct. Now let's examine our "bad table" component.
// src/app/badTable/badTable.component.ts
import { CommonModule } from '@angular/common';
import { Component, Input, OnInit } from '@angular/core';
@Component({
selector: 'bad-table',
templateUrl: './badTable.component.html',
styleUrls: ['./badTable.component.css'],
standalone: true,
imports: [CommonModule],
})
export class BadTable implements OnInit {
@Input() issues: any;
selectDeselectAllIsChecked = false;
numCheckboxesSelected = 0;
checkedState: any;
ngOnInit() {
this.checkedState = new Array(this.issues.length).fill({
checked: false,
backgroundColor: '#ffffff',
});
}
getStylesTr(issue: any) {
return issue.status === 'open' ? 'openIssue' : 'closedIssue';
}
onClick(index: any, issue: any) {
if (issue.status === 'open') {
this.handleOnChange(index);
}
}
handleOnChange(position: any) {
const updatedCheckedState = this.checkedState.map(
(element: any, index: any) => {
if (position === index) {
return {
...element,
checked: !element.checked,
backgroundColor: element.checked ? '#ffffff' : '#eeeeee',
};
}
return element;
}
);
this.checkedState = updatedCheckedState;
console.log('handleOnChange', this.checkedState, position);
const totalSelected = updatedCheckedState
.map((element: any) => element.checked)
.reduce((sum: any, currentState: any, index: any) => {
if (currentState) {
return sum + this.issues[index].value;
}
return sum;
}, 0);
this.numCheckboxesSelected = totalSelected;
console.log;
this.handleIndeterminateCheckbox(totalSelected);
}
handleIndeterminateCheckbox(total: any) {
const indeterminateCheckbox = document.getElementById(
'custom-checkbox-selectDeselectAll'
);
let count = 0;
this.issues.forEach((element: any) => {
if (element.status === 'open') {
count += 1;
}
});
if (total === 0) {
(indeterminateCheckbox as any).indeterminate = false;
this.selectDeselectAllIsChecked = false;
}
if (total > 0 && total < count) {
(indeterminateCheckbox as any).indeterminate = true;
this.selectDeselectAllIsChecked = false;
}
if (total === count) {
(indeterminateCheckbox as any).indeterminate = false;
this.selectDeselectAllIsChecked = true;
}
}
handleSelectDeselectAll(event: any) {
let { checked } = event.target;
const allTrueArray: any = [];
this.issues.forEach((element: any) => {
if (element.status === 'open') {
allTrueArray.push({ checked: true, backgroundColor: '#eeeeee' });
} else {
allTrueArray.push({ checked: false, backgroundColor: '#ffffff' });
}
});
const allFalseArray = new Array(this.issues.length).fill({
checked: false,
backgroundColor: '#ffffff',
});
if (checked) {
this.checkedState = allTrueArray;
} else {
this.checkedState = allFalseArray;
}
const totalSelected = (checked ? allTrueArray : allFalseArray)
.map((element: any) => element.checked)
.reduce((sum: any, currentState: any, index: any) => {
if (currentState && this.issues[index].status === 'open') {
return sum + this.issues[index].value;
}
return sum;
}, 0);
this.numCheckboxesSelected = totalSelected;
this.selectDeselectAllIsChecked = !this.selectDeselectAllIsChecked;
}
}
The first issue is that we're using the any data type for every variable, which lacks specificity. Secondly, the use of checkedState seems suboptimal. While we do need to store the list of selected items, storing the backgroundColor doesn't seem appropriate.
The total amount of code for such a small feature seems excessive, indicating that the code is not optimized.
Additionally, the logic for implementing indetermination is handled with native DOM methods and doesn't directly involve Angular. Overall, this code appears to be at a junior level and has room for significant improvement. Now, let's examine the HTML.
<table class="table">
<thead>
<tr>
<th>
<input
class="checkbox"
[type]="'checkbox'"
[id]="'custom-checkbox-selectDeselectAll'"
[name]="'custom-checkbox-selectDeselectAll'"
[value]="'custom-checkbox-selectDeselectAll'"
[checked]="selectDeselectAllIsChecked"
(change)="handleSelectDeselectAll($event)"
/>
</th>
<th class="numChecked">
<span *ngIf="numCheckboxesSelected"
>Selected {{ numCheckboxesSelected }}</span
>
<span *ngIf="!numCheckboxesSelected">None</span>
</th>
</tr>
<tr>
<th></th>
<th>Name</th>
<th>Message</th>
<th>Status</th>
</tr>
</thead>
<tbody>
<ng-container *ngFor="let issue of issues; index as index">
<tr class="getStylesTr(issue)" [ngStyle]="checkedState[index]">
<td>
<input
*ngIf="$any(issue).status === 'open'"
class="checkbox"
[type]="'checkbox'"
[id]="'custom-checkbox-' + index"
[name]="issue.name"
[value]="issue.name"
[checked]="checkedState[index].checked"
(change)="handleOnChange(index)"
/>
<input
*ngIf="issue.status !== 'open'"
class="checkbox"
[type]="'checkbox'"
disabled
/>
</td>
<td>{{ issue.name }}</td>
<td>{{ issue.message }}</td>
<td>
<span class="openCircle" *ngIf="issue.status === 'open'"></span>
<span class="resolvedCircle" *ngIf="issue.status !== 'open'"></span>
</td>
</tr>
</ng-container>
</tbody>
</table>
Here, we can observe unnecessary quotes, the use of any, and markup that could be simplified.
Refactoring State
Just a reminder, if the code is extremely poor and you understand the feature thoroughly, it might be simpler for you to completely re-implement the feature from scratch instead of attempting to rewrite the existing code.
However, in this instance, we aim to rewrite the code rather than starting from scratch to demonstrate the correct approach.
The initial issue we encounter is the absence of types in our application. Let's address this by defining an interface to represent our issues.
// src/app/goodTable/issue.interface.ts
export interface IssueInterface {
id: string;
name: string;
message: string;
status: 'open' | 'resolved';
numEvents: number;
numUsers: number;
value: number;
}
Here, I've specified all the possible fields to ensure clarity on the properties we can access.
// src/app/app.component.ts
export class AppComponent {
issues: IssueInterface[] = [
...
]
}
Now we've added a data type for our list of issues.
// src/app/goodTable/goodTable.component.ts
export class GoodTable implements OnInit {
@Input({ required: true }) issues!: IssueInterface[];
...
}
We've marked our issues input as required and set the correct data type.
Now, it's time to establish a better state for our application. In addition to the list of issues, I'd like to store some view data. For each issue, we need to track the isSelected property to determine if the element is selected or not. Thus, I propose creating the following structure:
{
"1": {"isSelected": false},
"2": {"isSelected": true}
}
With this structure, we can easily determine whether the issue is selected or not at any given time.
interface IssueEntriesInterface {
[key: string]: { isSelected: boolean };
}
...
export class GoodTable implements OnInit {}
Here, we've created an interface for this data structure. I've chosen to store all data within signals as it enables us to utilize computed properties and write less code overall.
export class GoodTable implements OnInit {
...
issuesSig = signal<IssueInterface[]>([]);
issueEntriesSig = signal<IssueEntriesInterface>({});
ngOnInit() {
this.issuesSig.set(this.issues);
this.issueEntriesSig.set(this.convertIssuesToEntries(this.issues, false));
}
convertIssuesToEntries(
issues: IssueInterface[],
isSelected: boolean
): IssueEntriesInterface {
const entries = issues.map((issue) => [
issue.id,
{ isSelected: issue.status === 'open' ? isSelected : false },
]);
return Object.fromEntries(entries);
}
}
Here, I've created two signals: one for the list of issues and another for the view object containing the isSelected property. Additionally, I've introduced the convertIssuesToEntries function, which generates our issueEntriesSig by mapping through each issue.
Refactoring a List
Let's begin by enhancing our HTML, starting with rendering a list.
<ng-container *ngFor="let issue of issuesSig()">
<tr
[ngClass]="{
openIssue: issue.status === 'open',
resolvedIssue: issue.status === 'resolved'
}"
[ngStyle]="{
backgroundColor: issueEntriesSig()[issue.id].isSelected
? '#eeeeee'
: '#ffffff'
}"
(click)="selectRow(issue.id)"
>
<td>
<input
class="checkbox"
type="checkbox"
[checked]="issueEntriesSig()[issue.id].isSelected"
/>
</td>
<td>{{ issue.name }}</td>
<td>{{ issue.message }}</td>
<td>
<span
[ngClass]="{
openCircle: issue.status === 'open',
resolvedCircle: issue.status === 'resolved'
}"
></span>
</td>
</tr>
</ng-container>
We've simplified the markup by utilizing ngClass and ngStyle. We eliminated additional functions by incorporating logic inline. Moreover, we've leveraged our signals and rendered the isSelected property based on our issue ID. This approach has resulted in cleaner and more readable code. Additionally, we've handled the backgroundColor inline instead of maintaining it as a separate state, as it was previously done.
Refactoring State Change
Now, it's time to implement our selectRow function. We trigger it for any element inside the row. This is why we removed the change event from the checkbox because we want a single handler for the entire row.
selectRow(issueId: string): void {
const updatedIssueEntry = {
...this.issueEntriesSig()[issueId],
isSelected: !this.issueEntriesSig()[issueId].isSelected,
};
const updatedIssueEntries = {
...this.issueEntriesSig(),
[issueId]: updatedIssueEntry,
};
this.issueEntriesSig.set(updatedIssueEntries);
}
The purpose of the selectRow function is to update the isSelected property in our view state. Here, we prepare the key that we want to update and replace it in our issueEntries signal.
.resolvedIssue {
color: #bdbdbd;
cursor: not-allowed;
pointer-events: none;
}
Additionally, we aim to prevent any events for our resolved issues. Since they have the .resolvedIssue class, we can achieve this with CSS by adding the pointer-events style.
Here's how our view state looks after selecting a row.
Refactoring Totals
Computed properties are incredibly useful for refactoring the totals feature because they enable us to create a new signal based on other signals.
export class GoodTable implements OnInit {
...
totalSelectedSig = computed(
() =>
Object.values(this.issueEntriesSig()).filter(
(issueData) => issueData.isSelected
).length
);
}
Here, we've created a signal based on our issueEntries, which will be automatically updated when the state changes. It now holds the total count of selected items.
<th class="numChecked">
<span *ngIf="totalSelectedSig() > 0"
>Selected {{ totalSelectedSig() }}</span
>
<span *ngIf="totalSelectedSig() === 0">None</span>
</th>
In the header, we've updated the markup to render the total count from the signal.
Refactoring Toggle All
Next, let's update our HTML to accommodate the changes in our toggle all logic.
<th>
<input
class="checkbox"
type="checkbox"
[checked]="totalSelectedSig()"
(change)="selectAll($event)"
/>
</th>
Here, we utilized our totalSelected signal and call a selectAll function when clicked. Now, let's create this function.
selectAll(event: Event): void {
const target = event.target as HTMLInputElement;
const updatedIssueEntries = this.convertIssuesToEntries(
this.issuesSig(),
target.checked
);
this.issueEntriesSig.set(updatedIssueEntries);
}
Here, we've utilized our existing convertIssuesToEntries function to update our view state and toggle all issues at once.
Refactoring Indeterminate
The final feature we want to modify is our indeterminate functionality. We need to highlight the checkbox as partially selected based on the number of selected elements.
indeterminateSig = computed(() => {
const totalOpenedIssues = this.issuesSig().filter(
(issue) => issue.status === 'open'
).length;
return (
this.totalSelectedSig() < totalOpenedIssues && this.totalSelectedSig() > 0
);
});
To address this issue, we can create one more computed signal to hold this value. If some elements are selected but not all, we want to set the value of the signal to true.
<th>
<input
class="checkbox"
type="checkbox"
[indeterminate]="indeterminateSig()"
[checked]="totalSelectedSig()"
(change)="selectAll($event)"
/>
</th>
Inside the HTML, we've added an indeterminate attribute that simply renders this signal.
As you can see, the entire project functions as before, but our code is now much cleaner and easier to maintain.
Want to conquer your next JavaScript interview? Download my FREE PDF - Pass Your JS Interview with Confidence and start preparing for success today!
📚 Source code of what we've done