illogical code - execute cypher query

[bazo]

i've been using neo4jphp for a while and have noticed that it contains some very strange code or ways how to do things that don't make any sense to me.

for example running cypher query:

$query = new \Everyman\Neo4j\Cypher\Query($this->client, $cypher, $vars);

ok, so Query needs client to be able to execute itself, which is strange by itself.

then we have a client method, $this->client->executeCypherQuery(Query $query); which takes a query argument.

this creates a circular dependency. the Query object is practically useles because you still need to have the client at hand.

why not just have a method $this->client->executeCypherQuery($cypher, $vars); ?

this is the most recent weirdness i have found, but i remember there being other similarly fashioned constructions.

is there a specific reason for this, that i don't understand?

[jadell]

The Client is the point of entry for talking to the server. It mainly instantiates the Command objects that pass their data to the Transport. It also keeps track of centrally needed objects, like the EntityCache, the Transport and some others. Another of its responsibilities is looking up existing retrieved Nodes/Relationships in the EntityCache in order to avoid requests to the server.

Almost every object that is not the Client takes the Client as one of its constructor objects. This makes it so that you can deal with the object, and not have to keep track of the Client alongside.

From your example, if the query did not have a handle to the Client, it would mean passing the Client and the Query object together into any other methods that might use them. It's not a circular dependency, since only the Query depends on the Client, not the other way around (you can execute a query without passing it to the Client by calling the Query's getResultSet method.) One could write an extension of the Query object that performs the same action the Client does (in this case, instantiating an ExecuteCypherQuery Command object and executing it.) But this would make the Query object another point of entry for talking to the server, which is the Client's job. And the Command object needs the Client anyway, so the Query object would still need a handle to it.

You ask what the utility of the Query class is, and at the moment, there is not a lot of functionality going on there. But a Query is a separate semantic idea from the database connection or any other objects that might be involved in executing the query and marshaling its results. My personal style is to encapsulate distinct entities in separate classes.

Mainly, the Client is there so that other objects don't have to know anything about how the server communication happens (you can see this in the unit tests, where many tests use a mock Client object instead of a real one.) The objects keep their own handle to the Client so that users don't have to pass around a Client object everywhere; the objects are mostly self-contained and merely proxy any server communication to the Client.

All this makes the Client somewhat of a "god-object" that I am looking to refactor at some point in the future. Part of that refactoring is making objects less dependent on the Client. Another part of the refactoring is to making Commands know less about the Node/Relationship entity layer, and deal more purely with PHP primitives.

This stuff would probably be best written down in some sort of architecture document on the wiki.

Is this architecture preventing you from accomplishing a goal?

[bazo]

no it doesn't but it seems wrong to me, the code contains a lot of cross cutting concerns

Query could loose the reference to client and also the method getResultSet(). Keeping Query as semantic envelope is ok, then $this->client->executeCypherQuery($query) gives much more sense than calling getResultSet on the Query object; i guess it's called decoupling

i'm sure dropping the client handle could be done for many other objects. Node forexample.

Compare this code:

$node = $client->makeNode()->save() or $node = new Node($client);

vs

$node = new Node(); $client->save($node);

I hope that you agree the second approach is much cleaner.

the code responsible for saving node, retrieving query results and what not should left only in the responsibility of the client. This would lead to simpler and more cleaner and clearer architecture. There's only 5 entities that need to be handled by the client:

1. nodes
2. relationships
3. queries
4. indexes
5. commands

That's just 5 methods in Client class, and a lot of duplicate code removed from the entity classes.

You say the Client dependency of these classes is to prevent passing around the client object but i still need it pass almost everywhere, mostly to create nodes, relationships, indexing and finding nodes. You even need it to pass it to a query, so this point doesn't hold;

As a side Node extends from PropertyContiner which has Client dep. in constructor, which doesn't make sense at all. A property container should just contain properties, the db stuff should be in Node

hope it makes sense :)

[jadell]

I don't really see how

 $node = new Node();
 $client->save($node);

saves much time or mental overhead compared to

 $node = new Node($client);
 $node->save();

There is an actual difference, which is that in the second case, after I have instantiated a Node, I can pass that through any other methods or architecture layers I want to, and will always be able to save it, without also needing to pass around a Client.

I understand what you are saying. Some classes, especially the Client, have too many concerns, have their fingers in too many other object and should be simplified. I agree there is some cleanup work to be done.

At this point in time, what you are suggesting would become a major break in the API. If you'd like to come up with an alternative architecture for a future version of neo4jphp, I would be happy to work on that with you, review any pull requests you submit, and discuss the relative merits of one way versus another.