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.

4
  • 1
    @MZawg Just to let you know. I upvoted here as that's the right place to ask IMO. Commented Feb 24, 2020 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
    Commented Feb 24, 2020 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.
    Commented Mar 9, 2020 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
    Commented Mar 10, 2020 at 8:21

1 Answer 1

3
+50

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

6
  • 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
    Commented Mar 10, 2020 at 9:16
  • while looking at your code i also noticed that you might not completely understand how rxjs and it's observables work. Check out learnrxjs.io for a nice list of it's operators/ best practices and common pitfalls
    – fox125
    Commented Mar 10, 2020 at 9:33
  • Thank you for the advice, I’ll go read this now. Appreciate it!
    – MZawg
    Commented Mar 10, 2020 at 9: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
    Commented Mar 10, 2020 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
    Commented Mar 10, 2020 at 13:23

Your Answer

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

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