CODEWITHSUNDEEP
angulartypescriptrxjs
Funky
Funky

Reputation: 13602

Typescript - Is calling another method inside the .subscribe() good or bad practice?

This works but I'm a little nervous about it as I'm not sure if it is just a hack or good practice. Can someone please let me know if this is a hack or good practice??

I have done it this way to ensure that the first method runs completely before starting the second method.

If this is bad practice, which I'm sure it is please provide a better way to implement it.

I have one method which makes a call to a HTTP GET method then in the subscribe binds it and calls another method which calls another service using that data. 

 private bind(): void {

        this.Service.get(this.Id)
        .catch(this.logger.log)
        .subscribe(
        (customers: PersonalInfo[]) => { 

        this.customers = customers;               
                this.bindContactInfo();           
        }
        );
    }

private bindContactInfo():void{       

        this.Service.getContactInfo(this.Id)
        .catch(this.logger.log)
        .subscribe(   

        (contactInfo: ContactInformation[]) => {

         // stuff in here
        }
        );
        }

Upvotes: 0

Views: 2889

Answers (1)

Olaf Horstmann
Olaf Horstmann

Reputation: 16882

In general you can do this, I think in a Service this is okay to do it like this, however if your code is from a component and since you are asking for a "good practice", here are some options:

Option 1

You should try to avoid manual subscriptions, use .do(...) for those kind of operation. Also try to avoid "hard chaining" where you cannot use the parts separately, so try breaking up your streams into smaller ones, this makes it easier to test. With those points in mind I would write your parts like this:

private getPersonalInfo(): Observable<PersonalInfo[]> {
    return this.Service.get(this.Id)
        .catch(this.logger.log)
        .do((customers: PersonalInfo[]) => this.customers = customers);
}

private getContactInfo(): Observable<ContactInformation[]> {
    return this.Service.getContactInfo(this.Id)
        .catch(this.logger.log)
        .do((contactInfo: ContactInformation[]) => this.contactInfo = contactInfo);
}

private getData(): void {
    this.getPersonalInfo()
        .switchMap(() => this.getContactInfo())
        .subscribe();
}

Option 2

Since I'm assuming you are using this in a component, I would try to avoid things like this.contactInfo = contactInfo all together and try to use | async in the template (given that I use components only to display information and not do any calculations there)

id$: ReplaySubject<string> = new ReplaySubject<string>(1); // this comes possibly from a service or from an @Input

personalInfo$: Observable<PersonalInfo[]> = this.id$
        .switchMap(id => this.Service.get(id))
        .catch(this.logger.log)
        .publishReplay(1)
        .refCount();

contactInfo$: Observable<ContactInformation[]> = this.id$  // or start it with this.personalInfo$.switchMapTo(this.id$) if you want to make sure that it is executed AFTER the personalInfo was fetched
        .switchMap(id => this.Service.get(id))
        .getContactInfo(this.Id)
        .catch(this.logger.log)
        .publishReplay(1)
        .refCount();

and in your template you would use it like this:

<div *ngFor="let personalInfo of personalInfo$ | async">
    ...
</div>

Option 3

Use some store-concept like ngrx - which probably will cause quite a few refactorings.

Upvotes: 3

Related Questions