Code seems repeated, except for the callback function

Question

I've written some JavaScript code:

 app.get('/offers/', function(req, res){
 var funcGetOffers = utils.funcGetOffers;
 var options = {top_sort: true};
 funcGetOffers(function(offers){
 res.render('offers', { offers:offers });
 }, options);
 });
 app.get('/api/get_offers', function(req, res){
 var funcGetOffers = utils.funcGetOffers;
 var options = {top_sort: true};
 funcGetOffers(function(offers){
 res.json(offers);
 }, options);
 });

The code is almost the same, and only the callback function is different. I want to get rid of the repeat code, but didn't find a good way.

funcGetOffers is a long function which gets a list of items from the database:

var funcGetOffers = function(callback, opt) {
 var order = [['updatedAt', 'DESC']];
 var where = {};
 if (opt && opt.top_sort) {
 order.unshift(['flag_top', 'DESC']);
 }
 var Offer = models.offer;
 Offer.findAll({order:order}).success(function(offers){
 // some calc with offers
 // callback
 callback(offers);
 });
}

---

Thank @Flambino make me more clear, I did some change to funcGetOffers to make it promisily, now the code:

var options = {top_sort: true};
utils.funcGetOffers(options).then(function(offers){
 res.render('offers', { offers:offers });
});
utils.funcGetOffers(options).then(function(offers){
 res.json(offers);
});

changed the funcGetOffers signature, use a bluebird to make it promisily.

utils.funcGetOffers(options).then(res.json) didn't work by the promise, but looks much better now!

Answer 1

You don't need the local funcGetOffers variable. Just call util.funcGetOffers directly.

You may extract the options to a var outside the request handlers, since it's the same in both places.

After that, there's not really any repetition left. Sure, the 2 request handlers each call utils.funcGetOffers, but the callbacks are so different that it'd be more complex to generalize them.

However, you can skip the callback function expression for the /api/get_offers handler, and simply use res.json directly.

var options = { top_sort: true }; // maybe call this offersOptions to make it clearer
app.get('/offers/', function(req, res){
 utils.funcGetOffers(function(offers){
 res.render('offers', { offers:offers });
 }, options);
});
app.get('/api/get_offers', function(req, res){
 utils.funcGetOffers(res.json, options);
});

I don't think it's necessary to do anything more elaborate than that.

---

And again, in utils.funcGetOffers you don't need all those local variables (you can access models.offer directly, and where isn't even used), and you can skip the success-callback, and just use the given one directly

var funcGetOffers = function(callback, opt) {
 var order = [['updatedAt', 'DESC']];
 if(opt && opt.top_sort) {
 order.unshift(['flag_top', 'DESC']);
 }
 models.offer.findAll({order:order}).success(callback);
}

---

In general, I'd recommend a bit more consistency. You obviously have a column named updatedAt (camelCase), but you apparently also have one called flag_top (underscored). Pick one way of naming things, and stick with it (for JavaScript, the convention is to use camelCase).

This extends to the options object (or opts, but there's no need to shorten it: Just call it options). There you have a key called top_sort, which should probably be camelCased - especially since you're not using it directly, but "translating" it to mean flag_top. In fact, it might be smarter to simply use the actual column name, rather than the - pretty ambuiguous - name top_sort.

And it'd be wise to handle errors too - not just success.

Lastly, there's really no need for the func prefix. If something's called getOffers it's implied that it's a function.

• Gohan: Thank you for your advise! very helpful! I tried Promise pattern, I think it makes the function looks better.