current community

your communities

more stack exchange communities

Stack Exchange
tour help
Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 2 down vote favorite

As a toy project, I started evaluating the Neo4J graph database and its Rest interface. I'm drying to write a simple graph visualization in Javascript. In daily business I'm a Java developer and maybe that's the reason why I'm not that happy with my current approach. In Java I would have written a Neo4J class, with inner (data) classes for Node and Relation as their id is tightly couple to the server (http://localhost:7474/db/data/node/0). Maybe they even would get a common parent class.

In Javascript I don't get away from my object oriented mind. I need some ideas in relation to Javascript best practice.

function Neo4J(server) {
    this.server = server;
}

function Node(connection, response) {
    this.id = connection.sanitizeId(response['self']);
    this.data = response['data'];
}

function Relation(connection, response) {
    this.id = connection.sanitizeId(response['self']);
    this.start = connection.sanitizeId(response['start']);
    this.end = connection.sanitizeId(response['end']);
    this.type = response['type'];
}

Neo4J.prototype = {
    sanitizeId: function (id) {
        return id.replace(this.server, "");
    },

    query: function (query, success) {
        return post(this.server + "/cypher", {"query": query, "params": {} }, success);
    },

    getAllNodes: function (callback) {
        var connection = this;
        this.query("MATCH n RETURN n", function (nodes) {
            $.each(nodes['data'], function (i, val) {
                callback(new Node(connection, val[0]));
            });
        });
    },

    getAllRelations: function (callback) {
        var connection = this;
        this.query("START r=rel(*) RETURN r", function (nodes) {
            $.each(nodes['data'], function (i, val) {
                callback(new Relation(connection, val[0]));
            });
        });
    },

    getNode: function (id, callback) {
        var connection = this;
        get(this.server + id + "/properties", function (data) {
            if (data == null) data = {data: {}};
            else data = {data: data};
            data['self'] = id;
            callback(new Node(connection, data));
        });
    },

    createNode: function (callback) {
        var connection = this;
        post(this.server + '/node', null, function (data) {
            callback(new Node(connection, data));
        });
    },

    setNodeProperty: function (node, key, value, callback) {
        put(this.server + node.id + '/properties/' + key, '"'+value+'"', function (data) {
            node.data[key] = value;
            callback(node);
        });
    },

    createRelation: function (start, target, type, callback) {
        var connection = this;
        post(this.server + start + '/relationships', {to: this.server + target, type: type}, function (data) {
            callback(new Relation(connection, data));
        });
    }

}

The post() and put() are just ajax calls to the Neo4J server.

share|improve this question
add comment

1 Answer

up vote 2 down vote

Interesting question,

The most striking thing is your repeated use of this pattern:

connection.sanitizeId(response['xxx']);

I dont believe sanitizeId should be a function of connection. Also, response is not a good name because it often is part of a response from the query and not the actual response. And, it is suggested that response.xxx makes more sense than response['xxx']. To that end, I feel that sanitizeId should be part of the response or data. Also, I would be tempted to call it simply sanitize.

function addSanitizer( data , server){
  data.sanitize = function( propertyName ){
    return this[propertyName].replace( server , "" );
  }
  return data;
}

Then you could do

function Node(response) {
    this.id   = response.sanitize( 'self' );
    this.data = response.data;
}

Or, if you agree that response might be misleading

function Node(node) {
    this.id   = node.sanitize( 'self' );
    this.data = node.data;  
}

Node with be called like this:

getNode: function (id, callback) {
    var connection = this;
    get(this.server + id + "/properties", function (data) {
        if (data == null) data = {data: {}};
        else data = {data: data};
        data['self'] = id;
        callback(new Node(this.addSanitizer( data, this.server ), data));
    });
},

From a style point:

  • You are skipping new lines on your if statements: if (data == null) data = {data: {}};, dont do that

  • Your code stretches quite a bit horizontally, but not too bad, feel free to to split up some of your longer lines into something more readable, this :

    createRelation: function (start, target, type, callback) {
    var connection = this;
    post(this.server + start + '/relationships', {to: this.server + target, type: type}, function (data) {
        callback(new Relation(connection, data));
    });
}

    could be 

    createRelation: function (start, target, type, callback) {
    var connection = this,
        data = {to: this.server + target, type: type};
    post(this.server + start + '/relationships', data , function (data){
        callback(new Relation(connection, data));
    });
}

The code looks all in all fine to me. For a Java developer you did good :P

share|improve this answer
    
Thanks for the feedback. I guess the if is more a religious discussion. If I add a line break, I also have to add braces and so I get 3-4 lines. Also a line length of 160 character is only an issue in web representation. But you are right, a final refactoring is missing anyway. –  mnhg May 21 at 4:43
    
I like the idea of the sanitizer, but have the feeling that this is a kind of over-engineered if I only need it for exactly one field? In my OO-mind I think the Neo4J(Server) class should be responsible for transforming an absolute server specific id to a relative id (and vice versa)? I really need to get used to bind this to the actual data. Btw. the response is called response because it is exactly the ajax rest response. In java this would be a constructor to create a Node from a JsonResponse. –  mnhg May 21 at 5:03
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.