3

I think I am just look for a bit of code review advice. It might possibly be a methodology question?

Essentially when I am pulling data (usually from a REST request), I generate a service, then inject HTTPclient of Angular common so I can use a simple GET to grab the data. Like below:

interface ReportContact{
  Email: string;
  Name: string;
  ReportName:string;
  ReportType:string;
  ReportFunction:string;
}

@Injectable({
  providedIn: 'root'
})


export class ReportingContactService {

  private readonly url = environment.url; 
  private _http:HttpClient;
  constructor(http:HttpClient) {
    //DI 
    this._http=http;
  }

    getReportContacts():Observable<ReportContact[]>{
    return this._http.get<ReportContact[]>(this.url);
  }
}

This is generally my go to when I am making a simple service. I then usually use this service in my component like below:

export class ReportingContactComponent implements OnInit {

  _service:ReportingContactService;
  data:ReportingContact[];

  constructor(service:ReportingContactService) {
    //DI service
    this._service = service;
   }

  ngOnInit() {
    this._service.getReportContacts().subscribe(
      data=> {
            for(var element of data){

            }
    });
  }

}

Now, where I was hoping for some review and pointers on is, should I now be mapping the return of that service to a class that I create as a model? I am used to the mindset of separating data type objects from the objects used in the rest of the application. So essentially in that for loop (of the component.ts), I would map to these new objects which look like below:

export class ReportingContact{
    Email: string;
    Name: string;
    ReportName:string;
    Type:string;
    Function:string;
}

Am I wasting my time and is this generally not the "norm"? Again, I just have always come from the mindset of separating, but the Angular framework (or maybe just TS and JS) has introduced new ways of thinking for me, so I like to be keeping to the generally accepted standards.

MZawg
  • 403
  • 4
  • 9
  • 1
    _@MZawg_ Just to let you know. I upvoted here as that's the right place to ask IMO. – πάντα ῥεῖ Feb 24 '20 at 18:56
  • @πάνταῥεῖ, thank you. I appreciate being directed to the best venue. As a post searcher, it also makes it easier for me to find what I need when looking. So I think its very important to correct stuff like that! – MZawg Feb 24 '20 at 18:57
  • 1
    not related to your question but you can add a private/protected/public keyword to the constructor injected parameters to make them automatically part of the class and simplify – C.M. Mar 09 '20 at 14:37
  • That depends whether the DTO matches your domain - if you just have the types supported by JSON (here an object containing strings), there's no need. If you had something more complex (e.g. there was a `number`/`string` that you wanted to convert to a `Date`, or you wanted accessors/methods on the domain object), then this should probably be done in the *service*. – jonrsharpe Mar 10 '20 at 08:21

1 Answers1

3

The short answer for this specific case would be: no there is no reason to map the (interface) result to a class. Since your class doesn't have any functions, doesn't do anything specific in it's constructor and only exposes the exact same properties the interface does, there is no added value in doing so.

There are however a few things you should consider:

1. bundle size

When compiled to javascript, interfaces will not generate any code while each class will result into a javascript prototype. Therefore a lot of Typescript classes will end up in a lot of javascript code (useless code if you end up only using them like interfaces)

Consider using the typescript playground to see what code typescript compiles to

Compiled interface and class

2. how javascript uses your types

While using interfaces/classes allows you to catch compile time code-errors, it does not guarantee you won't have any at runtime. For example if your http result doesn't fully comply with your interface ( e.g. the email property is missing) javascript won't care about that, resulting in an undefined reference error when you try to do something with it.

You could use your class contructor to make sure all properties are initiated correctly or provide a default value (example) Doing so will prevent these errors at runtime or enable you to handle them before they become an actual issue.

3. how you use your classes

In frontend (Angular) applications interfaces often times suffice as they are only meant as vessels to present data to your user. As your applications grow however, you often times find yourself in need of more and more client-side bussiness-logic. In these cases classes can be key to a domain driven client side architecture

fox125
  • 96
  • 3
  • Thank you, this is exactly what I was looking for. I just fell into the approach I’ve taken in many other application approaches I’ve seen, where they always talk about separation of like database objects for example. So, I was stuck in the mindset of doing that, without thinking of the added overhead. Thanks for the concise answer. – MZawg Mar 10 '20 at 09:16
  • while looking at your code i also noticed that you might not completely understand how rxjs and it's observables work. Check out https://www.learnrxjs.io/ for a nice list of it's operators/ best practices and common pitfalls – fox125 Mar 10 '20 at 09:33
  • Thank you for the advice, I’ll go read this now. Appreciate it! – MZawg Mar 10 '20 at 09:37
  • you sure were right. I had no idea how extensive RXJS was. I will be honest, I just setup the call to the web service based on a tutorial I ended up watching on YouTube. I am rather new to Angular and observables. However, if you don't mind, I did just now finish reading that entire link you sent and I am sure I have plenty to practice, but I am struggling to find the exact issue you are seeing in what I assume is where I subscribe. My apologies if its right in front of my face :(. – MZawg Mar 10 '20 at 11:51
  • you subscribe to an observable without storing the subscription, making it impossible to unsubscribe. when the observable is destoryed the subscription will remain in memory. All manual subscriptions to observables are therefore potential memory leaks. a good alternative is angular's async pipe. – fox125 Mar 10 '20 at 13:23
  • Thank you! You were exactly right, and I was doing this commonly in my components. – MZawg Mar 10 '20 at 15:28