1
\$\begingroup\$

I'm trying to display the fetched data on a table. I structured the data based on the columns table in the UI and stored them in an array, then *ngFor on the table rows on the frontend. Good or bad practice? Or are there other ways to do this in a much simpler way?

    getLicensesofDealer(id) {
        this._license.get_license_by_id(id).subscribe(
            (data: License[]) => {
                let counter = 1;

                // Structure data and store in an array
                data.forEach(l => {
                    const license = {
                        unique_key: l.licenseId,
                        count: counter,
                        key: l.licenseKey,
                        date: l.dateCreated
                    }
                    this.license_data.push(license);
                    counter++;
                })
                // Structure data and store in an array

            }
        )
    }
\$\endgroup\$
2
\$\begingroup\$

just my personal opinion, so pick what you like and ignore the rest. :-)

In general i think it is a good practice to use your "own" data structure in the frontend. if the backend changes their format, then you only have to change your mapping functions, and the rest could stay the same. If you use the backend format directly, then your code depends on that format. Means, if the backend changes a name, you would have to change that name all over your application...

Some points in general:

subscribe/unsubscribe
the first thing i would like to mention is, that if you subscribe in a component, then this subscription will be still active, after your component is long dead. Or to make it more explicit, when you "subscribe", then call the method "subscribe" on the observable. And you provide a (fat arrow) function to it. This function is then stored in the observable. Now, each time the observable emits a value, the function is executed. Because this function now "lives" in the observable, it does not get cleaned up when your component is destroyed. This can create memory leaks and/or unwanted behavior.

Therefor if your oberservable does not complete by itself (e.g. an http call, will only do one emit and then "complete") you should always clean up your subscriptions. This could be done by saving them all into a "Subscription" object.

private subscriptions: Subscription = new Subscription();
// ...
this.subscriptions.add(
    this.myObservable.subscribe( (...)=>{...})
)
this.subscriptions.add(
    this.myOtherObservable.subscribe( (...)=>{...})
)

ngOnDestroy(){
  this.subscriptions.unsubscribe();
}

forEach vs. map
forEach is used if you want to execute something for each element of an array. Map is used, if you want to change the format of each element. In your case you want to change the format, so i would prefer "map"

let counter = 1;
this.licence_data = data.map( el => {
   return {
     count: counter++,
     unique_key: el.licenceId,
     key: el.licenceKey,
     date: el.dateCreated
   };
}

Readability by extracting into methods
I personaly like it, when i don´t have to read 5 lines (often much more) of code to understand what happens here. Its much easier if a method is called and the name of the method gives me a good clue what happens in it. As a result, sometimes i don´t even have to look into the method, because the name alone tells me that the problem i am hunting is not there. OR the name gives me some context which allows me to understand the content of the method more easily.
And last but not least, splitting code into multiple methods means to separate the code into chunks which communicate with each other over interfaces. And that makes changes much easier, because, as long as i do not change the interface (the method signature), as long can i do changes without having to fear to break something outside. That makes refactorings much easier.

this._licence.get_licence_by_id(id).subscribe(
  (data: Licence[] => this.licence_data = this.mapToUIFormat(data)
);

this.mapToUIFormat(licences: Licence[]): MyUiFormat { // i would give that format an interface
    let counter = 1;
    return data.map(  licence => {
       return {
         count: counter++,
         unique_key: licence.licenceId,
         key: licence.licenceKey,
         date: licence.dateCreated
       };
    }

}

The advantage of this method is, that its a "pure" method. It only expects some input, and it returns an output that ONLY depends on the input. It also has no side effects, means it does not change anything else. That makes it easier to understand and very easy to test.

If you want you could also extract the mapping of a single licence into the ui format in a extra method. It depends on the complexity of the code, but in some circumstances it really helps to structure the code.

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ JanRecker I owe you one, This will help me a lot. Everything you said is so easy to understand and I thank you for that, I hope I could buy you a beer someday. Thanks Man! I really appreciate it. \$\endgroup\$ – Vhin Gabuat Feb 25 at 10:54

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.