HiveBrain v1.2.0
Get Started
← Back to all entries
patternjavascriptMinor

Node.js API Server for querying a Neo4j database

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
nodequeryingdatabaseneo4jforserverapi

Problem

I've written a server in Node.js that listens to http requests via Express.js and forwards requests to a Neo4j server. It's my first time working with Neo4j and Express.js. The code runs as intended but is an eyesore to read.

The Database has the following Nodes and Relationships:

(:User)-[:RANTED]->(:Rant)
(:User)-[:UPVOTED]->(:Rant)
(:User)-[:DOWNVOTED]->(:Rant)
(:Rant)-[:HAS_COMMENT]->(:Comment)
(:User)-[:COMMENTED]->(:Comment)
(:User)-[:UPVOTED]->(:Comment)
(:User)-[:DOWNVOTED]->(:Comment)


And the API simply supports creation of these nodes and relationships. Edit and Delete functionality is out of the project's scope.

NOTE: The server is meant for a simplistic social network for my final year university project. We are only being marked on the deliverables and not the code quality.

package.json

{
  "name": "Design-Rant-Server",
  "version": "0.1.1",
  "description": "Server layer between DR Database and Frontend",
  "main": "server.js",
  "dependencies": {
    "async": "*",
    "body-parser": "~1.0.1",
    "express": "~4.0.0",
    "neo4j-driver": "*",
    "object-checker": "^0.3.24",
    "validator": "^6.0.0"
  },
  "scripts": {
    "test": "mocha test",
    "start": "node server.js"
  },
  "author": "Paras DPain",
  "license": "MIT",
  "devDependencies": {
    "mocha": "^3.1.1"
  },
  "repository": {
    "type": "git",
    "url": "git+https://github.com/ParasDPain/DRServer.git"
  }
}


server.js

```
/** DR Starts server and API listeners
*
* Written By:
* Paras DPain
*
* License:
* MIT License. All code unless otherwise specified is
* Copyright (c) Paras DPain 2016.
*/
"use strict";

// REQUIRES
const express = require("express");
const bodyChecker = require('object-checker').bodyCheckMiddleware;
const bodyParser = require("body-parser");
const validator = require('validator');
const db = require('./api.js');
const checkerOptions = require('./checkerOptions.js')

// GLOBALS
var app = express();
var router = e

Solution

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 has two callback parameters, result and callback. A more descriptive name would perhaps be resultCallback and errorCallback. performVotes even has both callback and cb, which is very confusing to a reader!

Comments is a very useful tool to help readers understand particularly tricky code. They can however lead to less readable code when used too much. Obvious comments like // REQUIRES before a block of require calls are simply a nuance in my opinion.

I don't know if I'm missing something, but it seems like your routing comments are misleading. They all mention the prefix /api, but I don't see it anywhere in the code.

Assignment is not necessary when defining functions. You can drop var and = in the definition of performVotes.

In fact, don't use var at all. Since ES6, the new version of JavaScript, there are two new assignment statements const and let. let roughly corresponds to var, but with saner rules, while const is a whole new concept that should be used whenever reassignment isn't strictly required. Read this article if you're interested in why.


Sometimes it’s tempting to create an identifier to represent some data
and then use that identifier as a temporary place to store values in
transition from one representation to another.


For instance, you may be after a query string parameter value, and
start by storing an entire URL, then just the query string, then the
value. This practice should be avoided.


It’s easier to understand if you use one identifier for the URL, a
different one for the query string, and finally, an identifier to
store the parameter value you were after.


This is why I favor const over let in ES6. In JavaScript, const
means that the identifier can’t be reassigned.


[...]


var is now the weakest signal available when you define a variable
in JavaScript. The variable may or may not be reassigned, and the
variable may or may not be used for an entire function, or just for
the purpose of a block or loop.

Generally you should avoid global mutable state. rantCount can easily lead to race conditions and other hard to track bugs. I don't know Neo4j, but databases usually have some atomic increment feature to do globally unique ids you could use instead.

checkerOptions.js has lots of duplicated code. Say you want to make a change to allow usernames with upper case letters and numbers as well. You'll have to edit five different regexes! Instead, define a globals for username and id patterns and reuse them.

const USER_NAME_PATTERN = /^[a-z]+$/g;


Concerning these regular expressions, there's no need for the g flag in your case. It stores the index of each match to allow multiple matches on the same string, but since all your patterns are anchored with ^ and $, only one match is ever possible!

Code Snippets

const USER_NAME_PATTERN = /^[a-z]+$/g;

Context

StackExchange Code Review Q#143584, answer score: 3

Revisions (0)

No revisions yet.