Hot answers tagged

18

I wouldn't both prefixing 'internal' functions with an underscore. Instead, just put this in a module, and only export the functions you want to export. If you care about following the node style guide, you should know that node suggests ignoring Crockford's fetish for one var declaration. Note: this is pretty controversial. I'm not a fan of var ...


17

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


14

Code looks fine - but it strikes me as somewhat unnecessary. A promise can only be resolved/rejected once, so if you just keep the promise object itself around, its result is automatically cached within. I.e. if you call .then() on an already-resolved/already-rejected promise object, it'll just immediately invoke your handler(s) with whatever data it was ...


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

Your bitwise operations are fine, though the >>> right-shifts are unnecessary. @Lucien and @Corbin are right! The right-shift operators prevent (128 << 24) from being interpreted as a negative number. The rest of the code is fine too, but a bit long for my taste. Personally, I'd use a long regular expression to do all the parsing and ...


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


10

JavaScript function names should start with a lowercase character, and "staircase" is one compound word in English. Since every line is nearly the same, and only one character changes at a time, you should take advantage of that and use an array as a buffer. function staircase(n) { var line = Array(n + 1).fill(' '); line[n] = '\n'; for (var i = ...


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


9

The code looks pretty good. There are just a few stylistic things I would do differently. 1. Nested Describes You have quite a few tests which essentially test the same method of your model. Specifically, your tests which call functions like: saveLinkWithNoUser are all testing the Link.save. In cases like this, you can further organize your tests to be ...


9

Your parsing of IPv4 addresses is not incorrect, since it succeeds at parsing IPv4 addresses in dotted-quad notation as specified in RFC 810. However, you may be interested to know that the inet_aton() function in Unix is more lenient, supporting some unconventional forms: 1 : 0. 0. 0. 1 127.1 : 127. 0. 0. 1 192.168.1 : 192.168. 0. 1 ...


9

Very nice overall organization, use of promises, and use of constants where appropriate! Your code was a pleasure to read. General comments: You shouldn't implement convert(). It seems that you're receiving html from the WebSocket server. You should include a library to properly parse it and convert it to text. You already have cheerio, so you can use the ....


9

In modern versions of node.js (v6.0 or v4.x in strict mode) or when using an ES6 transpiler, you can use the ES6 class keyword: class Student { constructor(name, age, id) { // always initialize all instance properties this.name = name; this.age = age; this.id = id; } getStudentName() { return this.name; ...


9

It seems a little risky to me to just blindly use the language designation that the URL contains. If it's possible to trick Express into looking somewhere in your file system besides where you expect (perhaps with dots in the path), then you could have a security issue. It would be safer if you checked to see if it was one of a list of known-safe ...


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.


8

The Basics As you said, I think as each handler grows in size and the base handler adds more common features, the prototypes will pay off in spades. Honestly, the biggest readability issue for me is the require-and-instantiate style you're using. Is this common? I haven't seen it in any of the code for the tools I mentioned in my comment above. Compare ...


8

Fake for-loops and comprehensions At the beginning of your post, you say that you can't always achieve everything with looping in one line in CoffeeScript. When you say that, I assume you are taking about the each_file: each_file: (index = 0) -> length = @files.length file = @files[index] if index < length Types.read file, @...


8

A bunch of small things: In .addNewClient(), you can change this: var time = new Date().getTime(); to this: var time = Date.now(); Then, later in that same function you can use that value rather than calling Date.now() three more times. In .sendMessage(), you can change this: if( typeof(client) == typeof (" ")) to this: if( typeof(client) === "...


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


7

I do not see anything immediately wrong with your addUser() and getUser() implementations. Your hash() implementation could be improved in two areas though. Salt As already indicated by Bill Barry in the comments, your should use a better salt. Some rules to keep in mind when handling salts, Salts should be unique per password. Every password should have ...


7

First, there is a principle you should be reminded: Never deal with the error you don't know, just pop it up. Which means, if you can't deal with or don't know about the error passed in the callback, you just pass it to next callback, or throw it(A pattern of CHAIN OF RESPONSIBILITY)! For the errors you know how to deal with it, you can define a module ...


7

You can actually omit fs.exists(). The fs.stat() will return an error when the item you are testing is not there. You can scavenge through the err object that fs.stat() returns to see what error caused it. As I remember, when fs.stat() stats a non-existing entry, it returns an ENOENT, no such file or directory error. And so: var fs = require('fs') , ...


7

All of these Nested if Statements can definitely be changed into one level if statements if (commandList.indexOf('lockChat') !== -1) { if (lockChat === true) { bot.lockChat(room) } } if (commandList.indexOf('unlockChat') !== -1) { if (unlockChat === true) { bot.unlockChat(room) } } if (commandList.indexOf('setModerated') !== -...


7

Looks OK. Though I have to ask why you're using map, when you're not really mapping the array to a new array. Using reduce would be more semantically correct, but in this case even just replacing map with forEach would also be more accurate. (The difference is literally only skin-deep; there's zero change in overall functionality.) I might ask why an ...


7

One simple thing I see as a (small performance) improvement would be to store the length of your set of files. You have this tiny for: for (var i = 0; i < cfgfiles.length; i++) { Try to replace it with: for (var i = 0, length = cfgfiles.length; i < length; i++) { This speeds up the code because you don't have to access to the length property in ...


7

Use a linter The first thing I do when reviewing JavaScript code is run it through a linter. There several online linters, JSHint for example. JSHint will complain about ES6 features: 'arrow function syntax (=>)' is only available in ES6 (use 'esversion: 6'). To get around this, add this comment line at the top: // jshint esversion: 6 It's recommended ...


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


7

You can split each path at / using split and then use reduce() to build that object. var paths = [ 'RootFolder/FolderA/FolderB/FileA', 'RootFolder/FolderA/FolderB/FileB', 'RootFolder/FolderA/FolderB/FolderC/FileA', 'RootFolder/FolderA/FolderB/FolderC/FileB' ] var obj = {} paths.forEach(function(path) { path.split('/').reduce(function(...


7

That bus controller is too complicated; in general you should not be using new Promise(fn) unless you are wrapping a callback, and what you're doing is better done by returning a promise from the .then(). (When you return a value from myPromise.then() it gets passed into Promise.resolve(p), which if p is not a promise creates a new one which immediately ...


6

From a once over: Your indenting is a mix of 4 space and 8 space tabbing, please stick at all times to either 4 or 2 spaces. Looking at deserializeUser is aneurysm inducing. You have a ton of comments, which I like, authentication can always use tons of comment data[0] and data[1] both use a crucial magic constant, how about data[AUTHENTICATION] and data[...


6

Indentation The rightward drift in callback code makes code very hard to read—you're making the problem worse by choosing such large indentation. Most JS projects I've seen use 2 spaces. Personally, I find this much easier to read: module.exports = function() { // Local DB connection var mysql = require('mysql'); var theDB = mysql.createConnection({ ...


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