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.

asked Dec 28, 2016 at 8:04
\$\endgroup\$
2
  • 1
    \$\begingroup\$ You should at least be consistent with your layout, however you decide to do it. \$\endgroup\$ Commented Dec 28, 2016 at 10:15
  • \$\begingroup\$ @jonrsharpe it would be great if you can comment on the implementation.Thanks. \$\endgroup\$ Commented Dec 29, 2016 at 7:14

1 Answer 1

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...

answered May 8, 2017 at 22:35
\$\endgroup\$

Your Answer

Draft saved
Draft discarded

Sign up or log in

Sign up using Google
Sign up using Email and Password

Post as a guest

Required, but never shown

Post as a guest

Required, but never shown

By clicking "Post Your Answer", you agree to our terms of service and acknowledge you have read our privacy policy.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.