13

The power of the aggregation framework is it's ability to iterate over the dataset in various useful ways without incurring extra round trips between the database and the app. Your code uses one stage for aggregation (grouping by id1), and then jumps out of aggregation to iterate over the entire FUNDS collection for every user. That is very expensive, ...


10

According to www.lizard.ws the original's function cyclomatic complexity is 29 and for the second version is 22. Both numbers are usually considered high, and teams aim for much lower values (debatable what the good range is though and will see within this answer why). In order to reduce it, one way is to encapsulate the if statements, among with removing ...


8

I am not a mongoDB user but is there not some type of validation API, not sure if it can be used on schemes. If it can then maybe that is the better option for your code. The Question It have been getting complexity warnings and tried to rewrite it. Cyclomatic complexity (CC) is a count of the paths through a section of code. Note some code linters use ...


5

A short review; Consider using JsHint Declare data with var, otherwise you are polluting the global namespace Consider err more often, the assumption that there will be no error will bite you Do not simply log to console.log, it is one of the most common bottlenecks Remove your commented out code, it keeps things clean My assumption is that The meta data ...


5

I would suggest to refactor the code to make it cleaner using a function that checks user level and limits function validateData(data) { switch(data.level) { case 0: case 1: return data.description || data.certifications.length > 0 || data.teaser || data.social.length > 0 || data.locationLat || data.locationLong || ...


4

It seems that you are storing the data with double links (article -> category AND category -> articles). I assume that you need to report on articles for a category. I would simple create an index on Category like this: var articleSchema = mongoose.Schema({ category: { type: mongoose.Schema.Types.ObjectId, ref: 'Category', index: true }, title: ...


3

One of the biggest things you can do to clean up you code is to simply remove the unnecessary if-else blocks. In many cases you have a return inside the first conditional, which means there is no reason for an else block at all. For example, you code rewrite like this: Student.findOne({parent_ids:parent_id},function(err,parentF){ if(err){ ...


3

There's nothing outstandingly wrong, there are some minor points that can be improved. The main concern that I have is the amount of logic happening inside the ProductRoutes file. Your invocation of the AWS library and the validate & saveImage functions can be stored inside reusable files that can be referenced anywhere. I normally would store the ...


3

After looking at your code for a while, I think I understood your requirements. They can be summarized in this table: Level Descrs Certs TSoc Other 1 0 0 no no 2 29 0 no yes 3 49 3 yes yes 4 79 5 yes yes That's the essence of your feature matrix, and that's ...


2

This is due to asynchronous JavaScript, and this is called Callback Hell. You can deal with it through some design patterns, the Zoltan's answer is a really good one (way better than mine). But I'll show you another here based on your original code. Named callbacks The main goal here is to reduce the indentations level to improve the code readability by ...


2

Yes you're right, there are better solutions. For example you could use promises instead of callbacks since mongoose operations can return promises according to the documentation. Your example with promises: var Pet = require("models/pet.js"); var User = require("models/user.js"); exports.register = function (req, res, next) { var user = new User({ ...


2

The nested loops perform books.length * favorites.length iterations (up to 4 million). Instead build a dictionary of id -> rating, so total iterations max is a sum of lengths. var favRatings = {}, i, len; for (i = 0, len = favorites.length; i < len; i++) { var fav = favorites[i]; favRatings[fav.bookId] = fav.myRating; } for (i = 0, len = books....


2

Use Promises. Promises will help with all the indentation, and make your code more clean and understandable in the future (believe me, you'll have no idea of what you were trying to do if you read this code in a couple months) Break up your code in functions. Things like accessing Google Book API shouldn't be in the same place as retrieving data from MongoDB....


2

Interesting question, It seems you are using a ton of closures. 13 out of 15 function definitions are anonymous functions. I cant find a single one that is capturing anything useful since you have a ton of global variables on top. I would simply declare every one of those functions as a named function, it should already help a ton. And then indeed build ...


2

I don't quite understand why you need to save notifications in the first place. It seems you could just find them on the fly when the user GET's them. That said, here's a few comments: 1: It would be nice if you had split your app into smaller building blocks. Then you could write something like this: Comment.create(newComment) // ... .then(...


2

Rewrite Despite you explicitly asked for Promises, I'll show you my solution without nested callbacks. It was made simply by extracting those callbacks to standalone functions and applying few "best practices" to the code. Before the code itself, few remarks about it: Just like your code, it assumes availability of these 5 variables or objects: Parent, ...


2

Is this slowing down your app by a significant amount? If not, I wouldn't worry about it. Other than that I would consider removing the custom error handling, and rather pass it on to some other more general error handler at the end of your middleware stack (just call next(err)), and maybe make the Not Found error a 404 status code.


2

Well, the code is fairly simple, but you've made it complex due to a bad design. I would encourage the following: Use Promises. Mongoose implements promises and so does express, so most of the times you can avoid using callbacks. Also implement your own helpers with promises. Further more you can read up on Async/Await too. Read up on ES6. Use Arrow-...


2

In general I would say the code looks fine as far as functionality, though there are a few specific points which I will mention below with more detail. The broadest critique I have is that there are some variables declared with var or let that could use const since they are never re-assigned - for instance validator, ext, params, request, rejected, accepted ...


2

Paradigm shift: Table-driven methods Once logic becomes complex enough, you may find it easier to manage the rules from a data structure than from code. Here's how I picture that working for you here. Disclaimer: I made some assumptions about your business processes that may not be correct. Definitely review this code for correctness, and maybe rewrite it ...


1

You can use the forEach function like this const allPosts = []; Author.find({}, function(err, users) { if (err) throw err; users.forEach(function(user) { const a = user.posts; a.forEach(us => allPosts.push(us)); }); }); Now the allPosts array will have all the posts.


1

First of all, it appears that Item.findOne already returns a promise. You don't need to wrap it again in a promise. Simply use that same promise as the return value for your array.every. Now as far as I understand your problem, you just need to hit the db for every element in item and resolve the promise belongTo returns if every element belongs to user. I ...


1

Your modified code is better, but you can simplify this method even more. If you don't return anything from a function in a promise, it will be resolved with no value. There's no need to do return Promise.resolve(). .catch(error => next(error)) is equivalent to .catch(next). Code Tells You How, Comments Tell You Why. Don't clutter your code with ...


1

So I kind of figured this out. The db connector can be passed around like just another variable. Silly I didn't realise this before. I created a new file to abstract the model. dbOperations.js exports.addToDB = async function (model, dataObj, listName, res) { model.update({ userid: dataObj.userID }, { $addToSet: { listName: { ...


1

As I see there is about 60 properties for each item in Wynncraft DB. I see two ways to improve your design: Distribute all properties across multiple schemas: basic item information (name, category, etc), defense information, bonus damage information and other. In this case those schemas will be related to each other. Maybe you don't even need to store all ...


1

You have created a good model overall and I can only suggest you some consideration. Instead of using, created & lastModified. I would recommend you to use timestamp option in mongoose. const GAPFApplicationSchema = new Schema({ facultyId: { type: Number, index: true }, status: { type: String, enum: ["SUBMITTED", "BUDGET_ALLOCATED"] }, ...


1

The big question to ask is: what happens when it doesn’t work? (For whatever reason) In your case, the user will probably think it worked, only to find out later that it didn’t and will most likely think “stupid app, I just changed this”. You may be okay with it. I wouldn’t. The people paying me probably wouldn’t either. And what a blessed career you’ve ...


1

For the most part this looks pretty good, there are only a few things which I would recommend changing. As Igor Soloydenko recommends, get rid of the comments which do nothing but restate the code. Though comments are certainly not a bad thing, they should instead be used to explain why some code is the way it is if another programmer in 6 months (you) ...


1

Why aren't using mongoose .populate() method instead? Refer to the Mongoose documentation for Population here


1

First, using new RegExp on user input is a bit like running eval on user input (though not nearly as bad - usually). If it can be avoided, it should be. In this case, if I pass .*? as the partName, the regex will be /^.*?$/i which will match every entry in the database, or I could pass ( which would result in the function throwing an error. This can be ...


Only top voted, non community-wiki answers of a minimum length are eligible