5
\$\begingroup\$

I have a table post with a many-to-many relationship with the tag and product tables.

  • post \$\rightarrow\$ tag
  • post \$\rightarrow\$ product

How can I make this faster?

For example, if I have post, I can get similar posts similar by tags and products:

Post.php

<?php

namespace Ranger\AppBundle\Entity;

use Doctrine\ORM\Mapping as ORM;
use Doctrine\Common\Collections\ArrayCollection;

/**
 * Post
 *
 * @ORM\Table(name="post")
 * @ORM\Entity(repositoryClass="Ranger\AppBundle\Entity\PostRepository")
 * @ORM\HasLifecycleCallbacks()
 */
class Post
{
    CONST SHOW_PER_PAGE = 3;
    CONST SIMILAR_POST_MAX = 3;

    /**
     * @var string
     *
     * @ORM\Column(name="title", type="text", nullable=false)
     */
    private $title;

    /**
     * @var string
     *
     * @ORM\Column(name="content", type="text", nullable=true)
     */
    private $content;

    /**
     * @var boolean
     *
     * @ORM\Column(name="enabled", type="boolean", nullable=false)
     */
    private $enabled;

    /**
     * @var boolean
     *
     * @ORM\Column(name="keep_on_top", type="boolean", nullable=false)
     */
    private $keepOnTop;

    /**
     * @var \DateTime
     *
     * @ORM\Column(name="published", type="datetime", nullable=false)
     */
    private $published;

    /**
     * @var \DateTime
     *
     * @ORM\Column(name="created", type="datetime", nullable=false)
     */
    private $created;

    /**
     * @var \DateTime
     *
     * @ORM\Column(name="updated", type="datetime", nullable=true)
     */
    private $updated;

    /**
     * @var integer
     *
     * @ORM\Column(name="id", type="integer")
     * @ORM\Id
     * @ORM\GeneratedValue(strategy="IDENTITY")
     */
    private $id;

    /**
     * @var \Doctrine\Common\Collections\Collection
     *
     * @ORM\ManyToMany(targetEntity="Ranger\AppBundle\Entity\Tag", inversedBy="post")
     * @ORM\JoinTable(name="post_tag",
     *   joinColumns={
     *     @ORM\JoinColumn(name="post_id", referencedColumnName="id")
     *   },
     *   inverseJoinColumns={
     *     @ORM\JoinColumn(name="tag_id", referencedColumnName="id")
     *   }
     * )
     */
    private $tag;

    /**
     * @var \Doctrine\Common\Collections\Collection
     *
     * @ORM\ManyToMany(targetEntity="Ranger\AppBundle\Entity\Product", inversedBy="post")
     * @ORM\JoinTable(name="post_product",
     *   joinColumns={
     *     @ORM\JoinColumn(name="post_id", referencedColumnName="id")
     *   },
     *   inverseJoinColumns={
     *     @ORM\JoinColumn(name="product_id", referencedColumnName="id")
     *   }
     * )
     */
    private $product;

    /**
     * @var \Doctrine\Common\Collections\Collection
     *
     * @ORM\ManyToMany(targetEntity="Ranger\AppBundle\Entity\Category", inversedBy="post")
     * @ORM\JoinTable(name="post_category",
     *   joinColumns={
     *     @ORM\JoinColumn(name="post_id", referencedColumnName="id")
     *   },
     *   inverseJoinColumns={
     *     @ORM\JoinColumn(name="category_id", referencedColumnName="id")
     *   }
     * )
     */
    private $category;

    /**
     * @var \Doctrine\Common\Collections\Collection
     *
     * @ORM\ManyToMany(targetEntity="Ranger\AppBundle\Entity\Agency", inversedBy="post")
     * @ORM\JoinTable(name="post_agency",
     *   joinColumns={
     *     @ORM\JoinColumn(name="post_id", referencedColumnName="id")
     *   },
     *   inverseJoinColumns={
     *     @ORM\JoinColumn(name="agency_id", referencedColumnName="id")
     *   }
     * )
     */
    private $agency;

    /**
     * @var \Ranger\AppBundle\Entity\Image
     *
     * @ORM\ManyToOne(targetEntity="Ranger\AppBundle\Entity\Image")
     * @ORM\JoinColumns({
     *   @ORM\JoinColumn(name="image_id", referencedColumnName="id")
     * })
     */
    private $image;

    /**
     * Constructor
     */
    public function __construct()
    {
        $this->tag = new ArrayCollection();
        $this->product = new ArrayCollection();
        $this->category = new ArrayCollection();
        $this->agency = new ArrayCollection();
    }


    /**
     * Set title
     *
     * @param string $title
     * @return Post
     */
    public function setTitle($title)
    {
        $this->title = $title;

        return $this;
    }

    /**
     * Get title
     *
     * @return string 
     */
    public function getTitle()
    {
        return $this->title;
    }

    /**
     * Set content
     *
     * @param string $content
     * @return Post
     */
    public function setContent($content)
    {
        $this->content = $content;

        return $this;
    }

    /**
     * Get content
     *
     * @return string 
     */
    public function getContent()
    {
        return $this->content;
    }

    /**
     * Set enabled
     *
     * @param boolean $enabled
     * @return Post
     */
    public function setEnabled($enabled)
    {
        $this->enabled = $enabled;

        return $this;
    }

    /**
     * Get enabled
     *
     * @return boolean 
     */
    public function getEnabled()
    {
        return $this->enabled;
    }

    /**
     * Set keepOnTop
     *
     * @param boolean $keepOnTop
     * @return Post
     */
    public function setKeepOnTop($keepOnTop)
    {
        $this->keepOnTop = $keepOnTop;

        return $this;
    }

    /**
     * Get keepOnTop
     *
     * @return boolean 
     */
    public function getKeepOnTop()
    {
        return $this->keepOnTop;
    }

    /**
     * Set published
     *
     * @param \DateTime $published
     * @return Post
     */
    public function setPublished($published)
    {
        $this->published = $published;

        return $this;
    }

    /**
     * Get published
     *
     * @return \DateTime 
     */
    public function getPublished()
    {
        return $this->published;
    }

    /**
     * Set created
     *
     * @param \DateTime $created
     * @return Post
     */
    public function setCreated($created)
    {
        $this->created = $created;

        return $this;
    }

    /**
     * Get created
     *
     * @return \DateTime 
     */
    public function getCreated()
    {
        return $this->created;
    }

    /**
     * Set updated
     *
     * @param \DateTime $updated
     * @return Post
     */
    public function setUpdated($updated)
    {
        $this->updated = $updated;

        return $this;
    }

    /**
     * Get updated
     *
     * @return \DateTime 
     */
    public function getUpdated()
    {
        return $this->updated;
    }

    /**
     * Get id
     *
     * @return integer 
     */
    public function getId()
    {
        return $this->id;
    }

    /**
     * Add tag
     *
     * @param \Ranger\AppBundle\Entity\Tag $tag
     * @return Post
     */
    public function addTag(Tag $tag)
    {
        $this->tag->add($tag);

        return $this;
    }

    /**
     * Remove tag
     *
     * @param \Ranger\AppBundle\Entity\Tag $tag
     */
    public function removeTag(Tag $tag)
    {
        $this->tag->removeElement($tag);
    }

    /**
     * Get tag
     *
     * @return \Doctrine\Common\Collections\Collection 
     */
    public function getTag()
    {
        return $this->tag;
    }

    /**
     * Add product
     *
     * @param \Ranger\AppBundle\Entity\Product $product
     * @return Post
     */
    public function addProduct(Product $product)
    {
        $this->product->add($product);

        return $this;
    }

    /**
     * Remove product
     *
     * @param \Ranger\AppBundle\Entity\Product $product
     */
    public function removeProduct(Product $product)
    {
        $this->product->removeElement($product);
    }

    /**
     * Get product
     *
     * @return \Doctrine\Common\Collections\Collection 
     */
    public function getProduct()
    {
        return $this->product;
    }

    /**
     * Add category
     *
     * @param \Ranger\AppBundle\Entity\Category $category
     * @return Post
     */
    public function addCategory(Category $category)
    {
        $this->category->add($category);

        return $this;
    }

    /**
     * Remove category
     *
     * @param \Ranger\AppBundle\Entity\Category $category
     */
    public function removeCategory(Category $category)
    {
        $this->category->removeElement($category);
    }

    /**
     * Get category
     *
     * @return \Doctrine\Common\Collections\Collection 
     */
    public function getCategory()
    {
        return $this->category;
    }

    /**
     * Add agency
     *
     * @param \Ranger\AppBundle\Entity\Agency $agency
     * @return Post
     */
    public function addAgency(Agency $agency)
    {
        $this->agency->add($agency);

        return $this;
    }

    /**
     * Remove agency
     *
     * @param \Ranger\AppBundle\Entity\Agency $agency
     */
    public function removeAgency(Agency $agency)
    {
        $this->agency->removeElement($agency);
    }

    /**
     * Get agency
     *
     * @return \Doctrine\Common\Collections\Collection 
     */
    public function getAgency()
    {
        return $this->agency;
    }

    /**
     * @return Image
     */
    public function getImage()
    {
        return $this->image;
    }

    /**
     * @param Image $image
     * @return Post
     */
    public function setImage($image)
    {
        $this->image = $image;

        return $this;
    }



    /**
     * @ORM\PrePersist
     */
    public function setEnabledAfterCreate()
    {
        $this->enabled = true;
    }

    /**
     * @ORM\PrePersist
     */
    public function setCreatedAfterCreate()
    {
        if(!$this->getCreated())
        {
            $this->created = new \DateTime();
        }
    }

    /**
     * @ORM\PreUpdate
     */
    public function setUpdatedAtValue()
    {
        $this->updated = new \DateTime();
    }

}

PostRepository.php

<?php

namespace Company\AppBundle\Entity;

use Doctrine\ORM\EntityRepository;

/**
 * Class PostRepository
 * @package Company\AppBundle\Entity
 */
class PostRepository extends EntityRepository
{   

    public function getSimilarPost(Post $post)
    {
        $qb = $this->createQueryBuilder("p")
            ->select('p')
            ->setParameter('post', $post)
            ->setMaxResults(Post::SIMILAR_POST_MAX)
        ;

        /** @var Tag $tag */
        foreach ($post->getTag() as $tag)
        {
            $id = 'tag'.$tag->getId();

            $qb->orWhere(":$id MEMBER OF p.tag AND p != :post")
                ->setParameter($id, $tag);
        }

        /** @var Product $product */
        foreach ($post->getProduct() as $product)
        {
            $id = 'product'.$product->getId();

            $qb->orWhere(":$id MEMBER OF p.product AND p != :post")
                ->setParameter($id, $product);
        }

        return $qb->getQuery()->getResult();
    }
}
\$\endgroup\$
  • \$\begingroup\$ Welcome to Code Review! It would be helpful if you edited your question to say what in particular you'd like reviewed (or if all feedback is welcome). Asking how to do it better is vague. You did tag with performance so if you'd like it to be faster, note that in your question. \$\endgroup\$ – SuperBiasedMan Sep 22 '15 at 15:29
  • \$\begingroup\$ Maybe look into Criteria + matching(). A lot neater then using the lower level query builder. \$\endgroup\$ – Pinoniq Oct 12 '15 at 14:31
3
+50
\$\begingroup\$

In Post, most setters have a fluent interface, returning $this, that allow chained calls.

However, you break the consistency with a few setters, for example setEnabledAfterCreate. This inconsistency can be confusing, as users of the class will have to reference the implementation to know which setters can be chained and which cannot.

Similarly, it's odd that closely related operations like addTag and removeTag, don't have a symmetric implementation: addTag returns $this but removeTag returns nothing. The same goes for the corresponding methods for product, category, agency.

How can I make this faster?

I cannot see anything. The conditions you have seem all necessary. They are on foreign keys, so they must be already indexed. They are != conditions, no LIKE. I don't see how this can get any faster.

| improve this answer | |
\$\endgroup\$
2
\$\begingroup\$

Documentation

You're overusing documentation comments on code that doesn't need them to the point where it's becoming bad-looking fluff. For example, you have a lot of comments like this:

/**
 * Set enabled
 *
 * @param boolean $enabled
 * @return Post
 */

The above comments are then being used on incredibly simple methods, like this:

public function setEnabled($enabled)
{
    $this->enabled = $enabled;

    return $this;
}

These comments aren't needed because your code is self-documenting. Just by looking at the code, I can tell exactly what it's doing.

If you really want to use documentation comments, then you need to add are more useful description than "Set enabled", "Get content", or "Get title", and describe the arguments too, rather than just listing them.


Nitpicks

The following piece of code confused me, because it doesn't seem like it should run/compile:

CONST SHOW_PER_PAGE = 3;
CONST SIMILAR_POST_MAX = 3;

I was under the impression that constants were written like this, even in classes:

const SHOW_PER_PAGE = 3;
const SIMILAR_POST_MAX = 3;

Other than those few things, this looks fine to me. If there's anything specific you want me to cover just mention it in the comments and I'll see what I can do.

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ Thanks for your review! About comment This is PHPDoc comment, it's helper for IDE and generate documentation. About const You can use CONST like a const and others (private, protected etc.) in this way. Just a code style (I know, it isn't good and I replaced this). \$\endgroup\$ – Max Lipsky Oct 8 '15 at 6:31
1
\$\begingroup\$

I'm sorry to say that I haven't used doctrine my self, so you need to translate some of my response to make it really useful, but there might be some changes which would effect efficiency and speed. Mostly related to accessing the database behind your code.

Change order of tables being read

In general when reading from a database, it is better to read from the smallest table first. I'm not sure if this applies to your case, but if having a product database of several million, and a tag database of a few hundred, you should read first from the tag database and then include the product when you have limited based on the tags.

In other words, instead of looping through millions of products deselecting those matching a given tag from a few hundred tags, then loop through tags and find matching product. Way faster.

Group "member of" queries, if possible

When building your query you might end up with multiple orWhere's, and that could sever the execution speed of your query. If possible you should try to group these so that you'll end up with a query more similar to: or (tag1, tag2, tag3) member of p.product and p != :post (or reversed?) instead of or (tag1 member of p.product and p!= :post) or (tag2 member of p.product and p!= :post) or (tag3 member of p.product and p!= :post).

This would also allow for lesser checks against the p != :post. This check (or a variation thereof) could also possibly be added before the tags and product subqueries if any of those are present, since p == :post would falsify all subqueries.

Avoid unneccessary joins

You are linking category and product which could possibly affect your performance, as each you want either one, the other is joined in. Don't know your requirements, but in some cases it is better to postpone that join until it is actually needed. Some ORM's does handle this gracefully, don't know if yours does.

| improve this answer | |
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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