# Tag Info

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

6

After I examine the codes you have written, I could see some things that are not rightly done. Below are the few things I highlighted: The structuring of the codebase is not okay hence it can be maintainable and scalable if the codebase grows. The RESTful API naming convention is not an acceptable one. Endpoints of any REST API should contain only resources ...

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

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

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')...

5

Error handling You await inside an async function, but without a try/catch surrounding it. So, if Article.findAndCountAll rejects, you'll get an unhandled rejection (which is deprecated in Node) and no response will be sent. Default destructuring and variable names const { page } = req.params; // current Page const { size } = req.params; // items per page ...

5

Test case structure It doesn't seem like the code totally "lacked effort and structure" but the structure, at least of the test cases, could be better. Since all three describe() blocks contain a test that calls getLatlonFromPostcode so there could be a single describe() block with three it() blocks to test the various aspects of how the function ...

5

The code could be improved with a few tweaks, but it's not bad, and the overall structure looks just fine to me. Error handling is in the wrong place Some of your route handler are async, and inside them you await. Whenever you have a Promise, make sure to handle possible errors properly. Although the functions outside of your router handlers handle errors - ...

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

I am unsure if this is the correct way to do this (set nonces on each get) Yes, it is Remember that nonces must be regenerated for every page request and they must be unguessable. In terms of best practises, couple of things I'd recommend: Move your nonce manipulation into it's own middleware Cache your page content, don't read it from disk every time ...

4

Hopefully the response by James is sufficient for your concern about best practices. I did notice some redundancy in that snippet of Javascript. Specifically, the line that handles adding the nonce attribute to the script and style tags could be simplified: let newHTML = html.replace(/<script/g, '<script nonce="'+nonce1+'"').replace(/<...

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

The general rule is that you should never incorporate user-controlled input into a string that will be interpreted by a computer system without escaping it first. Specifically, at a quick glance, it's obvious that… Your exec() call is vulnerable to command-line injection: exec('ping -c 2 ' + req.body.address, …) Your SQL query is vulnerable to SQL ...

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

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