Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

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!

share|improve this question
    
You could post that update as an answer, or as a new question if you'd like it reviewed further. We no longer encourage appending updated code based on answer reviews. –  Jamal Jun 16 at 18:53

1 Answer 1

up vote 6 down vote accepted

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.

share|improve this answer
    
Thank you for your advise! very helpful! I tried Promise pattern, I think it makes the function looks better. –  Gohan Jun 16 at 18:33

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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