Is it poor practice to use very similar for loops in two places with Angular 6?

user_10322211 0
Viewed 105

I'm generating two lists for a roster.

One list to show the current members of that year, and a second list if people have been excused in that year.

I noticed that I have two of the same for loops except one prints out all of the true Boolean values, and one the false. Is there a better way (or a method of some sort) to print out the people who are excused and not excused?

<h2>Roster {{year-1}}-{{year}}</h2>

<div *ngFor="let k of peoples">
  <div *ngFor="let a of k.people; let j = index">
    <div *ngIf="k.year == year && k.people[j].excused == false">
      {{k.people[j].firstName}} {{k.people[j].lastName}}
    </div>
  </div>
</div>

<h2>Excused</h2>

<div *ngFor="let k of peoples">
  <div *ngFor="let a of k.people; let j = index">
    <div *ngIf="k.year == year && k.people[j].excused == true">
      {{k.people[j].firstName}} {{k.people[j].lastName}}
    </div>
  </div>
</div>
angular typescript
3 Answers

It would be better to use it this way without additional *ngIf.

As mentioned in comments it would be better to prepare to separete arrays for data in the component and do it once after data updated. But since the context of usage is unknown this looks like a better solution. You need to avoid *ngIf if you can filter data in array to prevent from unnecessary work another structure directive in template. It always gives you perfomance advanateges.

In your component you can define filter function:

getExcused(isExcused: boolean) {
    return this.peoples
        .filter(k=>k.year === this.year)
        .map(k=>k.people)
        .filter(p=>p.excused === isExcused);
}

Then in template: 

<h2>Roster {{year-1}}-{{year}}</h2>
<div *ngFor="let a of getExcused(false)">
    {{ a.firstName }} {{ a.lastName }}
</div>
<h2>Excused</h2>
<div *ngFor="let k of getExcused(true)">
    {{ k.firstName }} {{ k.lastName }}
</div>

If your component uses change detection strategy on push it won't make any perfomance issues.

user_65500920

Maybe I'am wrong, but isn't a pipe the prefered way of doing this ? Or better, you could chain 2 pipes !

<div *ngFor="let k of peoples| yearPipe: year | excusedPipe: no">

      {{k.people[j].firstName}} {{k.people[j].lastName}}

</div>

<h2>Excused</h2>

<div *ngFor="let k of peoples| yearPipe: year | excusedPipe: yes">

      {{k.people[j].firstName}} {{k.people[j].lastName}}

</div>

@Pipe({ name: 'yearPipe' })
export class YearPipe implements PipeTransform {
  transform(allPeople: People[], year: string) {
    return allPeople.filter(person => person.year == parseInt(year));
  }
}

@Pipe({ name: 'excusedPipe' })
export class ExcusedPipe implements PipeTransform {
  transform(allPeople: People[], excused: string) {
    if (excused === 'yes'){
       return allPeople.filter(person => person.excused);
    }else{
       return allPeople.filter(person => !person.excused);
    }
  }
}

The only tricky part is that the parameter to the pipe is allways string, or that is what I find in docs in Angular.io

Edit : refer to this stackblitz example ( Chained Pipes example ): https://stackblitz.com/edit/angular-rb5vmu

user_35585730

Its up to you how you want to structure your code, but if you want to avoid this, you could do something like this

html

<div *ngfor="let exused of isExcused(true)">
  {{excused.firstName}} {{excused.lastName}}
</div>

<div *ngfor="let exused of isExcused(false)">
  {{excused.firstName}} {{excused.lastName}}
</div>

ts

isExcused(excused) {
  let arr = [];
  for (let i = 0; i < this.peoples.length; i++) {
    if(peoples[i].year === this.year) { // it's better to have this check here
      for (let j = 0; j < this.peoples[i]people.length; j++) {
        if (peoples[i].people[j].excused === excused) {
          arr.push(peoples[i].people[j]);
        }
      }
    }
  }
  return arr;
}

But this is a very ugly solution. You should do something like this:

html

<div *ngfor="let exused of excusedList">
  {{excused.firstName}} {{excused.lastName}}
</div>

<div *ngfor="let unexcused of unexcusedList">
  {{unexcused.firstName}} {{unexcused.lastName}}
</div>

ts

ngOnInit() {
  this.excusedList = isExcused(true);
  this.unexcusedList = isExcused(false);
}

because it looks like your lists wont be updated on the fly. Also, you could use something .forEach() or .filter instead of for loops to help make the code not look so blocky.

user_89477480
Related