Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Readability #81

Open
drymek opened this issue Dec 22, 2014 · 2 comments
Open

Readability #81

drymek opened this issue Dec 22, 2014 · 2 comments
Labels

Comments

@drymek
Copy link

drymek commented Dec 22, 2014

It is quite hard to understand the current structure of the Specification. When you look at:

$spec = Spec::andX(
    Spec::eq('ended', 0),
    Spec::orX(
        Spec::lt('endDate', new \DateTime()),
        Spec::andX(
            Spec::isNull('endDate'),
            Spec::lt('startDate', new \DateTime('-4weeks'))
        )
    )
);

return $this->em->getRepository('HappyrRecruitmentBundle:Advert')->match($spec);

it's quite similar to Doctrine version (in the context of readability). I think it could be improved by such creation Specifications:

$spec = (new Property('ended'))->equals(0)->and(
    (new Property('endDate'))->lessThan(new \DateTime())->or(
        (new Property('endDate'))->isNull()->and(
            (new Property('startDate'))->lowerThan(new \DateTime('-4weeks'))
        )
    )
);

advantage is that you can read it:
Property ended is equals to "0" and property end date i less than DateTime (or $now = new \DateTime) (...). But it also has disadvantages:

  • Property class is a little bit virtual,
  • there is no new property at all,
  • it doesn't have to be a property,
  • implementation may seems tricky
  • and probably something else I did not notice

Recipe

  • Introduce new Property class:
class Property
{
    use Conditions;

    public function __construct($name)
    {
        $this->name = $name;
    }
}
  • Adding this trait to each filter (Equals, LessThan, isNull etc) and to Property class:
trait Conditions
{
    protected $propertyName;

    public function equals($value)
    {
        return new Equals($this->propertyName, $value);
    }

    public function lessThan($value)
    {
        return new LessThan($this->propertyName, $value);
    }

    public function isNull()
    {
        return new IsNull($this->propertyName);
    }
}
  • Adding this trait to LogicX class:
trait LogicSpecification
{
    public function and($value)
    {
        return new AndX($this, $value);
    }

    public function or($value)
    {
        return new OrX($this, $value);
    }
}

after correcting any typos it should work. Voilà.

Conclusion

This may not be the best solution, or it may be even worse than the existing one. I would like to ask you to comment on readability and/or alternative ideas.

@Nyholm
Copy link
Member

Nyholm commented Mar 17, 2015

Thank you for this issue. I really like that the syntax is easy to read.

I feel it is quite difficult to write with all those parentheses and to also get a idea over how the logic tree is built. Especially when we use a complex tree like in your example.

@peter-gribanov
Copy link
Member

Using fluent interfaces is not good.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants