Efficient method for building a promise object

Question

At the moment what I have is this :

    getData(user) {
     return this.$q.all({
        userInfo: this.$http.get(this.buildUrl('user.getinfo', user)),
        userTopArtists: this.$http.get(this.buildUrl('user.gettopartists', user)),
        userChart: this.$http.get(this.buildUrl('user.getWeeklyChartList', user))
            .then(resp => {
                let promises = [];
                let data = resp.data.weeklychartlist.chart;
                for (let i = data.length - 4; i < data.length; i++) {
                    promises.push(
                        this.$http.get(this.buildUrl('user.getWeeklyTrackChart', user) + `&from=${data[i].from}&to=${data[i].to}`)
                        .then(resp => {
                            return resp.data.weeklytrackchart.track.length;
                        })
                    );
                }

                return this.$q.all(promises);
            })
    }).then(resp => {
        return resp;
    }).catch(err => {
        this.$q.reject('Error' + err.status);
    })
}

But I'm thinking there may be a more functional approach to building this promise object, because a lot of code is getting repeated. So I tried to come up with a better solution:

    buildPromiseObj(target) {
     const methods = ['getinfo', 'gettopartists', 'getweeklychartlist'];
     let obj = {};
     for ( let method in methods ) {
        obj[`${methods[method]}`] = this.$http.get(this.buildUrl(`${target}.${methods[method]}`, target))
    }
}

Does this approach make sense and should I use it ?
You can see that the 3rd request I make in the first function has other nested requests. Is there any way I can integrate that in the approach I've written above ?

Personally I would keep as is, this `buildPromiseObj` has gained very little, and only provided you with another problem. — Keith, Nov 15 '17 at 14:03

Bergi · Accepted Answer · 2017-11-15T21:10:06.817

3

Yes, it's always a good idea to generalise like this. However:

Don't use for…in enumerations on arrays!
`${methods[method]}` is pointless use of template strings
your method/property names are inconsistent about capitalisation and prefix (user… vs get…)
Drop the pointless .then(resp => { return resp; })!
Your catch callback doesn't do anything. It should at least have returned the rejected promise - or simply thrown the error. Also you should not reject with strings.

Regarding the chart list, you can just append another then to the promise before calling all. Also you can greatly simplify the callback by using a more functional approach:

getData(user) {
    const methods = ['Info', 'TopArtists', 'WeeklyChartList'];
    let promises = {};
    for (let method of methods) {
        obj['user' + method] = this.$http.get(this.buildUrl('user.get'+method.toLowerCase(), target));
    }
    promises.userWeeklyChartList = promises.userWeeklyChartList.then(resp =>
         this.$q.all(resp.data.weeklychartlist.chart.slice(-4).map(val =>
             this.$http.get(this.buildUrl('user.getWeeklyTrackChart', user) + `&from=${val.from}&to=${val.to}`)
         ).then(resp =>
             resp.data.weeklytrackchart.track.length
         ))
    );
    return this.$q.all(promises).catch(err => {
        throw new Error(err.status);
    });
}

edited Nov 15 '17 at 21:10

answered Nov 15 '17 at 14:11

Bergi

630,263
148
957
1,375

Appreciat the in depth response, however I'm getting this wierd error, that I've been trying to solve for the past half hour `TypeError: Cannot read property 'chart' of undefined at promises.userweeklychartlist.promises.userweeklychartlist.then.resp` – As above so below Nov 15 '17 at 15:14
weeklychartlist is undefined here: let data = resp.data.weeklychartlist.chart; – Michael Rosefield Nov 15 '17 at 15:15
@TudorApostol Did you write a dot instead of the assignment? There is no `promises.userweeklychartlist.promises.userweeklychartlist.th‌en.resp` anywhere. The only place where a `.data` property is accessed is `resp.data.weeklychartlist` – Bergi Nov 15 '17 at 15:18
@MichaelRosefield Not sure what you mean, there is no `let data` statement in the code in my answer – Bergi Nov 15 '17 at 15:19
@Bergi I've reproduced the code exactly as you've written it, thats why I'm having problems with why this error would occur. – As above so below Nov 15 '17 at 15:21
@TudorApostol Make sure `resp.data` is what you expect and contains a `.weeklychartlist`. Notice that my code calls `this.buildUrl('user.getweeklychartlist', …)` instead of `this.buildUrl('user.getWeeklyChartList', …)`, as necessary for the generalisation. – Bergi Nov 15 '17 at 15:22
@Bergi It was a typo in `user.getweeklychartlist`. God, I love coding. – As above so below Nov 15 '17 at 15:59
@Bergi Any ideea why the nested requests are not getting called at the same time as the other requests ? – As above so below Nov 15 '17 at 16:53
@TudorApostol If by "nested requests" you mean those constructed inside `promises.userWeeklyChartList.then(…)`, then obviously that is because they have to wait for the weekly chart list to know what needs to be fetched. – Bergi Nov 15 '17 at 16:54
Let us [continue this discussion in chat](http://chat.stackoverflow.com/rooms/159083/discussion-between-tudor-apostol-and-bergi). – As above so below Nov 15 '17 at 16:55
@bergi, ah, sorry, I thought they were experiencing the issue with their original code – Michael Rosefield Nov 15 '17 at 16:59

Michael Rosefield · Answer 2 · 2017-11-15T17:03:51.457

As a first pass, here's what I would do with it.

        
const { $http: { get }, $q: { all } } = this,
      // convert { x: 'a', y: 'b' } to '&x=a&y=b'
      paramStr = obj => Object.entries(obj).map(([k,v]) => `&${k}=${v}`).join(), // You might not have Object#entries; it's new
      
      getUrl = (target, method, params={}) => this.buildUrl(`user.get${method}`, target) + paramStr(params),

      getData = user => all(Object.assign({},
        ...['info', 'topartists', 'WeeklyChartList'].map(m => get(getUrl(user, m)))
      ))
        .then(([ userInfo, userTopArtists, { data: { weeklychartlist: { chart } } } ]) => 
          all({
            userInfo, 
            userTopArtists,
            userChart: all([
                ...chart.slice(-4)
                  .map(({ from, to }) => 
                    get(getUrl(target, 'WeeklyTrackChart', { from, to }))
                      .then(({ data: { weeklytrackchart: { track: { length } } } }) => length)
                  )
              ])
          })
        )
        .catch(({ status }) => this.$q.reject(`Error${status}`));

I'm keeping your code functionally the same, though Bergi's recommendations are good ones.

Efficient method for building a promise object

2 Answers2