1
\$\begingroup\$

This is my first Angular2 component, which makes two api calls: the first call provide the input for the the second one.

The following is my attempt at this code.

@Component({
      selector: 'result-panel',
      templateUrl: 'panel.component.html',
      styleUrls: ['panel.component.css'],
      providers:[FindIpZoneDataService,ApiService]
    })

    export class PanelComponent
    {
      networkDetail:Array<Network>;
      query = "";
      datafetchservice:any;
      searching:boolean = false;
      rowCount:boolean = false;
      iperror :boolean = false;
      nwerror :boolean = false;
      userinputerror:boolean = false;
      constructor(private dataservice :FindIpZoneDataService){
            this.datafetchservice = dataservice;
      }

      getNetworkDetail(ip:string) {
        this.networkDetail = [];
        this.dataservice.getNetworkDetail(ip)
          .map((response) => {
            let res: Array<any> = [];
            let extattrs: Array<any> = [];
            res = response.json();
            res.forEach((detail) => {
              extattrs.push(detail.extattrs);
            });
            return extattrs;
          })
          .map((network:Array<any>) => {
            console.log(network);
            let result: Array<Network> = [];
            if (network) {
              network.forEach((detail) => {
                result.push(new Network(detail['SITE Name'].value,
                  detail['InfoSec Security Zone'].value,
                  detail['Network Security Zone'].value,
                  detail['Data Classification'].value));
              });
              return result;
            }
          })
          .subscribe(details => {
              this.networkDetail = details;
              this.rowCount = true;
              this.searching = false;
            },
            err => {
              //Valid Network not found
              this.handleNwServiceError(err);
            });
    }
      getIPNetwork(){
        this.datafetchservice.getNetwork(this.query)
          .map(response => response.json())
          .subscribe(result => {
              this.getNetworkDetail(result[0].network);
          },
            err => {
              //InValid IP
              this.handleIpServiceError(err);
            });
      }

      private handleNwServiceError(error: any) {
        //Observable.throw(error.json().error || 'Invalid Network - Server error');
        console.log(error);
        this.nwerror = true;
        this.rowCount = false;
      }

      private handleIpServiceError(error: any) {
        //Observable.throw(error.json().error || 'Invalid IP - Server error');
        console.log(error);
        this.iperror = true;
        this.rowCount = false;
        console.log('ip error');
      }

Please let me know how I can improve it.

\$\endgroup\$
  • 1
    \$\begingroup\$ You should at least be consistent with your layout, however you decide to do it. \$\endgroup\$ – jonrsharpe Dec 28 '16 at 10:15
  • \$\begingroup\$ @jonrsharpe it would be great if you can comment on the implementation.Thanks. \$\endgroup\$ – Shan Dec 29 '16 at 7:14
1
\$\begingroup\$
  1. Specify types explicitly whenever it's possible (including void for methods and FindIpZoneDataService for datafetchservice instead of any).

  2. Use const instead of let -- it will help detecting unintended assignments.

  3. Try not to move things around when it's not necessary. E.g. the following section is pushing one array's items into another.

let res: Array<any> = [];
let extattrs: Array<any> = [];
res = response.json();
res.forEach((detail) => {
  extattrs.push(detail.extattrs);
});
return extattrs;

Why do that if simple cast would work? Like this: <T[]>response.json() where <T> is whatever the specific type of the entity returned by the API.

  1. Stylistically, it's better to declare getIPNetwork() before getNetworkDetail() since the code is normally being read top-to-bottom.

  2. When one map() is immediately followed by another map(), they can always be merged into a single one. There is possibility of using flatMap() in addition to that, which would simplify some things but I need better understanding of your context...

    getIPNetwork(): void {
        this.datafetchservice
            .getNetwork(this.query)
            .map(response => response.json())
            .subscribe(
                result => this.getNetworkDetail(result[0].network),
                err => this.handleIpServiceError(err) // InValid IP
            );
    }
    
    getNetworkDetail(ip: string): void {
        this.networkDetail = [];
        this.dataservice
            .getNetworkDetail(ip)
            .map((response) => {
                return response
                    .json()
                    .map(detail => new Network(
                        detail['SITE Name'].value,
                        detail['InfoSec Security Zone'].value,
                        detail['Network Security Zone'].value,
                        detail['Data Classification'].value
                    ));
            })
            .subscribe(
                details => {
                    this.networkDetail = details;
                    this.rowCount = true;
                    this.searching = false;
                },
                err => this.handleNwServiceError(err) // Valid Network not found
            );
    }
    
  3. extattrs, nwerror, and similar names are meaningless/hard to decipher.

  4. TypeScript just like JavaScript follows camelCase naming convention. Stick to it. I.e. userinputerror should be userInputError, datafetchservice -- dataFetchService, and so on...

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.