18

You don't have to inject express into each modules. requiring express in each file will return the same instance. http://nodejs.org/docs/latest/api/modules.html#modules_caching Also, ask yourself who should be the one registering the router into the app. The app it self or the router ? For code reuse and clean separation you should let the app handle it. ...


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, ...


12

You can create a file db.js var mysql = require('mysql'); var settings = require('./settings.json'); var db; function connectDatabase() { if (!db) { db = mysql.createConnection(settings); db.connect(function(err){ if(!err) { console.log('Database is connected!'); } else { ...


8

How about a ternary operator? var redirect = req.from.redirect ? url.parse(req.from.redirect) : { protocol: 'http', hostname: config.domain, pathname: 'thanks' }; Looks cleaner in my opinion.


7

Okay I'm going to be a bit more comprehensive here since I've already read your code quite a few times, and I'm gonna assume that you are on node v6 The setup for the TodoControllerSpec feels kind of verbose. Can it be simplified? If you mean all the require calls you have to do...unfortunately not as far as I know. As a side note, I really like jest for ...


5

Interesting, I actually really like your constructor, it is neat and shows in one place what the class does. I am not sure I would call it SignupDoctorPostRequest, since it is not a request at all but processes a request. I would consider DoctorSignupModel or since it seems to leverage several models DoctorSignupMetaModel. As for your eager validation, I ...


5

There is a better way. If you wish to serve static files with express - it comes bundled with static middleware. To use it, you simply add this line before your routes: app.use(express.static(__dirname + '/public')); This may require slight adjustment to your paths but it replaces all those manual routes and is more efficient (caching, expiration etc). ...


5

I know you are generating data for the sake of completion but it does complicate things on what aspect of the code you'd like to focus on. But here are a couple thoughts. With Express, I often like to treat the routes as controllers in an MVC pattern. So any fetching of data and mutating data, I would create at least a model per resource to encapsulate that ...


5

Don't use callbacks, it's hard to read and maintain, the fs.readdir function will return a promise if you didn't provide a callback function. Use arrow functions when you could. Respond with an error instead of throwing (unless you have a catch wrapper middleware). Use property shorthands. You forgot req and res. Try this: const express = require('express')...


4

That does not look very DRY ( Dont repeat yourself ), not to mention that it does not look good that every function must be aware of the connection string. For updates, you could do something like this: exports.setPassword = function (id, password, callback) { genericUpdate('UPDATE users SET password=$1 WHERE id=$2', [password, id], callback ); }; ...


4

First, read about how .get() can chain multiple callbacks, then read how the sample code of Passport uses ensureAuthenticated as a callback. That should give you enough inspiration. Furthermore: English only in source code, especially for variables, even for comments Comments, your code could use more of them, and less, this comment is pretty useles: //...


4

What you're looking for is JSON.parse. It's not in the Node docs, because it's "lower" than that: Every modern javascript runtime has it (see MDN). Here's a simple function to read a file, parse it as JSON, and send it to a callback (all in Node) function readJSONFile(filename, callback) { require("fs").readFile(filename, function (err, data) { if(...


4

To answer your question: yes, I think it could be interesting as a stand-alone module. There's can be a lot of repetitive boilerplate involved in connecting a REST api to a CRUD backend, reducing that is certainly worth a module, in my opinion. Now to dive into the code... index.js if(!config.collection || !config.type){ //Raise an error here... ...


4

I would just name the variable for Bluebird promises as Promise as it mostly acts like the standard Promise object in ES6. That way, if you happen to run an ES6-compatible Node.js, you can just remove the import, and you'll be set. In the promise constructor, you need not do a return. The code will also feel awkward to a new developer, thinking Promise needs ...


4

There's a big problem here. getUserByEmail() can either return a promise or throw synchronously which is a bad design and very unfriendly for the caller. Instead, you want any synchronous errors to be returned as a rejected promise. The beginning of your code should be changed from this: // SERVICE / MODEL LOGIC getUserByEmail() { if (!this._model....


4

If you use 1 promise, you would encounter a long chain of then method and all of async operations would be executed in sequence style which increases response time. If you create 3 promises, all of that can be performed in parallel. So the answer is yes, you should implement with 3 promises. By the way, with help of bluebird library, You can write more ...


4

Utilize the query object, this type of querying is more suitable there than as params. Use object destructuring. Dry your code, see how we're mapping over [countryId, stateId] to check the validity of the input. Ideally, you would have been using a module like Joi for that. Maintain a happy path and break out of the function early when erroring. Use async/...


4

Ways of improving/optimizations: What fixApostrophes and fixDashes functions actually try to do is replacing a specific punctuation chars with respective HTML entities. Instead of those numerous horrifying while loops - a more optimized, concise and extendable approach would be to: compose a predefined replacement map (where keys are search patterns and ...


3

Heyo! This is pretty good, but doesn't have any password hashing (storing your passwords in plain text is bad). You might want to instead consider using an authentication library like either passportjs or stormpath. If you're really set on rolling your own auth stuff, you could use this project I wrote as an example (it's using the same tools you are): ...


3

Get rid of all those commas: i18n.init detectLngQS: "lang" ,ns: { namespaces: ['ns.common', 'ns.layout'], defaultNs: 'ns.common'} ,resSetPath: "./locales/__lng__/new.__ns__.json" ,... CoffeScript doesn't need them: i18n.init detectLngQS: "lang" ns: { namespaces: ['ns.common', 'ns.layout'], defaultNs: 'ns.common'} resSetPath: "./locales/...


3

Sorry to tell you, but unfortunately it is near impossible to dry html in itself if you want to keep the elements in the same way. As far as I can see, you already use only the minimum amount of elements. Move on, your code is fine


3

Use map/reduce Whenever you can, you should consider using map functions to transform data. or example, you transform a JSON items array, to a sites array. This is currently done like: var sites = []; for (var i = 0; i < json.items.length; i++) { var site = json.items[i]; sites.push({ title: site.name, icon: site.icon_url, ...


3

if(typeof req.body.data !== 'undefined' && req.body.data ... // to if(req.body.data && req.query.hash && typeof req.body.id) Assuming that false, 0, null and an empty string has no special meaning in this program, you can simply use loose comparison. Anything falsy (null, undefined, false, 0, NaN, empty string) will just fall off. ...


3

I only looked at the tests briefly, but they look good and I like your approach The following changes will help the readability of your main code: Prefer early returning guard clauses over if/else. It's easier to follow, uses less code, and avoids a level of indentation. Name concepts clearly and move them to helper functions, so that your main body ...


3

It's often a good idea to separate routes into their own files to get a better overview of the project. Try out express-generator and see how the example project is organized. Use a logging framework to log requests. I've used morgan which is really nice. Callbacks are like any other parameters and should be given a descriptive name. For instance, query ...


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

As for the jQuery/function usage, there is definitely room for improvement but it would be helpful to see an example HTML element that contains the article img, title, and url to understand the page structure a bit. I think your variable names are clear, but could be simplified. Also, it is common practice to use Camel Case with the first letter lowercase ...


3

A short review; Your database related functions should be separated, in my mind your socket methods should only do this: Normalize parameters your receive like with isCheckedNum Call a method to Create/Read/Update/Delete data (with a promise mechanism) Emit to rooms if succesful and needed Send back the appropriate HTTP response code with extra info if ...


3

This is missing the point. It is not about promises. You are right that if done correctly, you'll get a nice flat structure, but not if you inline all your code. This would be true if you weren't using promises as well. What you should do is split your code into smaller building blocks, and the nice structure will follow. Also, be wary of your variable ...


3

Not a node expert, but it looks like you cover the basics. CSRF protection via token is standard practice and very reliable. You are generating your token via a cryptographically secure random number generator. You are leaving it in the session and making sure it is present on all post requests. The only way an attacker can break it is via XSS attack, ...


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