Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

Sign up
Here's how it works:
  1. Anybody can ask a question
  2. Anybody can answer
  3. The best answers are voted up and rise to the top

Because it is a lot of code, I don't want to ask for details what is wrong, but want to ask about review what can be changed in approach to this.

This is controller that make query:

  • It grabs data within time range
  • Does aggregation so I get sum of some fields
  • Because there are some fields from different collections i.e leader_name, I do other queries
  • There is a $in: workers fragment in the $match part that also need workers to filter from a different query

getGroupInRange: function (req, gres) {

    console.log(req.params);

    Group.find({name: {$in: req.params.id.split(',')}})
      .populate('workers')
      .exec(function(err, found){

        if (err) return gres.serverError(err);

        var workers = [];

        found.forEach(function(x){
          x.workers.forEach(function(xx){
            workers.push(xx.id);
          });
        });

        workers = workers.map(function (x) {
          return new ObjectId(x);
        });



        Event.native(function(err, collection) {
          if (err) return gres.serverError(err);

          var query = [
            {
              $match:
              {
                probe_time:
                {
                  $gte: new Date(req.params.from),
                  $lt: new Date(req.params.to)
                },
                worker_id:
                {
                  $in: workers
                }
              }
            },
            {
              $group:
              {
                _id:{worker_id:"$worker_id", group:"$group"},
                "total_logged_time":{$sum: "$duration"},
                "total_break_time":{$sum: {$cond: [ {$eq:["$user.work_mode", 'break']},"$duration", 0] }},
                "total_nonidle_duration":{$sum: {$cond: [ {$eq:["$user.presence", 'active']},"$duration", 0] }},
                "total_idle_time":{$sum: {$cond: [ {$eq:["$user.presence", 'idle']},"$duration", 0] }},
                "total_pro_apps_time":{$sum: {$cond: [ {$eq:["$app_category", 'productive']},"$duration", 0] }},
                "total_nonpro_apps_time":{$sum: {$cond: [ {$ne:["$app_category", 'productive']},"$duration", 0] }},
                "status1":{$sum: {$cond: [ {$eq:["$user.work_mode", 'CUSTOM_1']},"$duration", 0] }},
                "status2":{$sum: {$cond: [ {$eq:["$user.work_mode", 'CUSTOM_2']},"$duration", 0] }},
                "status3":{$sum: {$cond: [ {$eq:["$user.work_mode", 'CUSTOM_3']},"$duration", 0] }},
                "status4":{$sum: {$cond: [ {$eq:["$user.work_mode", 'CUSTOM_4']},"$duration", 0] }},

                "leader_name":{$last:"$leader_name"},
              }
            },
            {
              $project:
              {
                "total_logged_time":1,
                "total_break_time":1,
                "total_nonidle_duration":1,
                "total_idle_time":1,
                "total_pro_apps_time": 1,
                "total_nonpro_apps_time" : 6388,
                "print_qty":1,
                "medium_print_qty":1,
                "medium_download_size":1,
                "effectivity":1,
                "status1":1,
                "status2":1,
                "medium_break_time": { $multiply:[3600, {$divide:["$total_break_time", "$total_logged_time"]} ] },
                "worker_name":"$_id.worker_id",
                "leader_name":1
              }
            },
            {
              $group:
              {
                _id:"$_id.group",
                data:{$push: "$$ROOT"}
              }
            }
          ];


          collection.aggregate(query).toArray(function (err, results) {
            if (err) return res.serverError(err);

            Worker.find({_id:{$in: workers}})
              .populate('leader')
              .exec(function (err, res) {

                results.map(function(x,y){
                  x.data.map(function (xx, yy) {

                    xx.leader_name = res.filter(function (x) {
                      return xx._id.worker_id.equals(x.id);
                    })[0].leader.fullname;

                  })
                });


                return gres.ok(results);
              });

          });
        });

      });
    }
share|improve this question
up vote 1 down vote accepted

First, you should fix your indentation and styling, I would use a tool like JSFiddle's Tidy function to clean up your code.


err:

The word you're looking for is error, no need to sacrifice readability for two characters.

.exec(function(err, found) {

Overriding err/error:

In the following code block, you overwrite the parameter err or error, this is bad practice, it'd be better off named something like error_two.

          collection.aggregate(query).toArray(function(error, results) {
              if (error) return res.serverError(error);

              Worker.find({
                      _id: {
                          $in: workers
                      }
                  })
                  .populate('leader')
                  .exec(function(error, res) {

In fact, you actually overwrite it three times!


Overriding x:

You override x in the following code block:

You should apply the same as before and name it xxx or something (because xx is already existent)

results.map(function(x, y) {
    x.data.map(function(xx, yy) {

        xx.leader_name = res.filter(function(x) {

y:

In the above point's code, you never actually use y, as it is the second parameter, it can be entirely removed, JavaScript does not require an explicit amount of parameters.


In total:

In total, it would look like:

getGroupInRange: function(req, gres) {

    console.log(req.params);

    Group.find({
        name: {
            $in: req.params.id.split(',')
        }
    })
    .populate('workers')
    .exec(function(error, found) {
        if (error) return gres.serverError(error);
            var workers = [];
            found.forEach(function(x) {
                x.workers.forEach(function(xx) {
                    workers.push(xx.id);
                });
            });
            workers = workers.map(function(x) {
                return new ObjectId(x);
            });

            Event.native(function(error, collection) {
                if (error) return gres.serverError(error);

                var query = [{
                    $match: {
                        probe_time: {
                            $gte: new Date(req.params.from),
                            $lt: new Date(req.params.to)
                        },
                        worker_id: {
                            $in: workers
                        }
                    }
                }, {
                    $group: {
                        _id: {
                            worker_id: "$worker_id",
                            group: "$group"
                        },
                        "total_logged_time": {
                            $sum: "$duration"
                        },
                        "total_break_time": {
                            $sum: {
                                $cond: [{
                                    $eq: ["$user.work_mode", 'break']
                                }, "$duration", 0]
                            }
                        },
                        "total_nonidle_duration": {
                            $sum: {
                                $cond: [{
                                    $eq: ["$user.presence", 'active']
                                }, "$duration", 0]
                            }
                        },
                        "total_idle_time": {
                            $sum: {
                                $cond: [{
                                    $eq: ["$user.presence", 'idle']
                                }, "$duration", 0]
                            }
                        },
                        "total_pro_apps_time": {
                            $sum: {
                                $cond: [{
                                    $eq: ["$app_category", 'productive']
                                }, "$duration", 0]
                            }
                        },
                        "total_nonpro_apps_time": {
                            $sum: {
                                $cond: [{
                                    $ne: ["$app_category", 'productive']
                                }, "$duration", 0]
                            }
                        },
                        "status1": {
                            $sum: {
                                $cond: [{
                                    $eq: ["$user.work_mode", 'CUSTOM_1']
                                }, "$duration", 0]
                            }
                        },
                        "status2": {
                            $sum: {
                                $cond: [{
                                    $eq: ["$user.work_mode", 'CUSTOM_2']
                                }, "$duration", 0]
                            }
                        },
                        "status3": {
                            $sum: {
                                $cond: [{
                                    $eq: ["$user.work_mode", 'CUSTOM_3']
                                }, "$duration", 0]
                            }
                        },
                        "status4": {
                            $sum: {
                                $cond: [{
                                    $eq: ["$user.work_mode", 'CUSTOM_4']
                                }, "$duration", 0]
                            }
                        },

                        "leader_name": {
                            $last: "$leader_name"
                        },
                    }
                }, {
                    $project: {
                        "total_logged_time": 1,
                        "total_break_time": 1,
                        "total_nonidle_duration": 1,
                        "total_idle_time": 1,
                        "total_pro_apps_time": 1,
                        "total_nonpro_apps_time": 6388,
                        "print_qty": 1,
                        "medium_print_qty": 1,
                        "medium_download_size": 1,
                        "effectivity": 1,
                        "status1": 1,
                        "status2": 1,
                        "medium_break_time": {
                            $multiply: [3600, {
                                $divide: ["$total_break_time", "$total_logged_time"]
                            }]
                        },
                        "worker_name": "$_id.worker_id",
                        "leader_name": 1
                    }
                }, {
                    $group: {
                        _id: "$_id.group",
                        data: {
                            $push: "$$ROOT"
                        }
                    }
                }];


                collection.aggregate(query).toArray(function(error_two, results) {
                    if (error_two) return res.serverError(error_two);

                    Worker.find({
                        _id: {
                            $in: workers
                        }
                    })
                    .populate('leader')
                    .exec(function(error_three, res) {
                        results.map(function(x) {
                            x.data.map(function(xx) {
                                xx.leader_name = res.filter(function(xxx) {
                                    return xx._id.worker_id.equals(xxx.id);
                                })[0].leader.fullname;
                            })
                        });
                        return gres.ok(results);
                    });
            });
        });
    });
}
share|improve this answer

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.