neo4j-php / php-cypher-dsl

A low-level query builder for Cypher written in PHP
MIT License
17 stars 5 forks source link

Is it possible to clean up the API? #3

Open kirkbushell opened 2 years ago

kirkbushell commented 2 years ago

I started using this package last few days, moving away from string concatenation, and I've found it really difficult to read the code once it's done. It seems to me the expectation around certain objects for arguments into the method calls is what makes it so verbose and difficult to read. It would be great if it could accept strings where necessary, at least then we could use object chaining whilst keeping the code readable.

Compare:

$deck = Query::node("Deck")->named($d = new Variable('d'));
$query = Query::new()->match($deck);

if ($format->raw() !== 'all') {
    $query->where($d->property('format')->equals(Query::parameter('format')));
}

$statement = $query->returning(Query::rawExpression('count(d) AS total'))->build();

if ($format->raw() !== 'all') {
    $result = $this->client()->run($statement, ['format' => $format->raw()]);
} else {
    $result = $this->client()->run($statement);
}

return $result->first()->get('total');

To:

if ($format->raw() !== 'all') {
    $result = $this->client()->run('MATCH (d:Deck) WHERE d.format = $format RETURN count(d) AS total', ['format' => $format->raw()]);
} else {
    $result = $this->client()->run('MATCH (d:Deck) RETURN count(d) AS total');
}

return $result->first()->get('total');

As it stands, I am really struggling to see the benefit of using this API to generate queries.

I would like to propose an API such as the following, which would help to move away from string concatenation (which is admittedly quite ugly):

$statement = Query::new()
    ->match('(d:deck)')
    ->where('d.format', '$format')
    ->return('count(d) AS total')
    ->build();
26 commented 2 years ago

Thank you for your feedback. I agree that the current API is rather verbose, but this is mostly out of necessity. The API needs to be this verbose to support the static type-safety this package provides. If the API were to use strings, IDE's would not longer be able to perform static analysis on the parameters, and you would lose much of the type-safety benefits this package offers. Secondly, Cypher is a rather complex query language that has many (sometimes unexpected) features we would like to support. Using strings would make this much harder.

That being said, things could definitely be done to improve the readability of the API and I will look into this. Thank you for taking the time to point this out.

kirkbushell commented 2 years ago

Thank you for your feedback. I agree that the current API is rather verbose, but this is mostly out of necessity. The API needs to be this verbose to support the static type-safety this package provides. If the API were to use strings, IDE's would not longer be able to perform static analysis on the parameters, and you would lose much of the type-safety benefits this package offers. Secondly, Cypher is a rather complex query language that has many (sometimes unexpected) features we would like to support. Using strings would make this much harder.

That being said, things could definitely be done to improve the readability of the API and I will look into this. Thank you for taking the time to point this out.

Could you elaborate on why the static type-safety is important? Why would strings make supporting other cypher features more difficult?

I'm wondering if maybe a simpler API could be supported along the current one?

kirkbushell commented 2 years ago

I can also see how both could be utilised:

Query::new()
    ->match('d', 'deck', ['format' => 'value'])
    ->where('d.label', '=', 'value')
    ->returns('d.label');

Each of those values/properties could effectively be static types, if that's important. In that way we kind of get the best of both worlds.

26 commented 2 years ago

Could you elaborate on why the static type-safety is important? Why would strings make supporting other cypher features more difficult?

I'm wondering if maybe a simpler API could be supported along the current one?

Static type-safety is import, because it give you more confidence in your code and it makes sure IDE's give relevant hints for composing your query.

However, I think it a good option would be to create a second API with less verbose syntax that supports the most common use-cases of Cypher, but it may be difficult to decide what and what not to support in this subset of the language.

26 commented 2 years ago

By the way, the current roadmap includes adding automatic identifier generation, which will also improve the syntax.