2
\$\begingroup\$

I have made a contract renewal system in Symfony and Doctrine, and it works, but I think it can be improved, but I make too many requests to the database.

These are my models:

Contract
------------
id
reference
....

ContractRenewal

class ContractRenewal
{
    /**
     * @ORM\Id()
     * @ORM\GeneratedValue()
     * @ORM\Column(type="integer")
     */
    private $id;

    /**
     * @ORM\ManyToOne(targetEntity="App\Entity\Contract", inversedBy="contractRenewalOld")
     */
    private $contractOld;

     /**
     * @ORM\ManyToOne(targetEntity="App\Entity\Contract", inversedBy="contractRenewalRenew")
     */
    private $contractRenew;

     /**
     * @ORM\ManyToOne(targetEntity="App\Entity\ContractRenewal", inversedBy="children")
     * @ORM\JoinColumn(name="parent_id", referencedColumnName="id",nullable=true)
     */
    private $parent;

    /**
     * @ORM\Column(type="datetime",nullable=true)
     */
    private $createdAt

I have done is put a renewal button on the contract I want to renew, and when I renew that contract I keep both the current contract and the new contract that has been created.

My intention is that when you consult a contract that may already be an old contract or one that is currently being renewed, i.e. active and was last renewed, you can see all the contracts related to that contract.

So when I consult any contract these are the requests I make to get all the renewals or contacts related to that contract.

/**
* renewals
*/
$renewalFindParent = $em->getRepository('App:ContractRenewal')->findActual($contract->getId());
$currentIsRenewal  = $em->getRepository('App:ContractRenewal')->findOneBy(array("contractOld" => $contract->getId()));
$renewals          = false;

if($renewalFindParent){
    if($renewalFindParent->getParent()){

        $renewals = $em->getRepository('App:ContractRenewal')->findByParent($renewalFindParent->getParent());

    }else{

        $newParent = $em->getRepository('App:ContractRenewal')->findOneBy(array("contractOld" => $renewalFindParent->getcontractRenew()));

        if($newParent){
            $renewals = $em->getRepository('App:ContractRenewal')->findByParent($newParent->getParent());
        }else{
            $renewals = $em->getRepository('App:ContractRenewal')->findBySingle($contract->getId());
        }

    }
}

Repository

namespace App\Repository;

use App\Entity\ContractRenewal;
use Doctrine\Bundle\DoctrineBundle\Repository\ServiceEntityRepository;
use Symfony\Bridge\Doctrine\RegistryInterface;


class ContractRenewalRepository extends ServiceEntityRepository
{
    public function __construct(RegistryInterface $registry)
    {
        parent::__construct($registry, ContractRenewal::class);
    }

    public function findByParent($parent)
    {
        return $this->createQueryBuilder('cr')
                ->where('cr.parent = :parent or cr.id =:parent or cr.contractOld = :parent')
                ->setParameter('parent', $parent)
                ->orderBy('cr.id', 'ASC')
                ->groupBy("cr.id")
                ->getQuery()
                ->getResult();
    }


     public function findActual($contract)
    {
        return $this->createQueryBuilder('cr')
                ->select("cr")
                ->where('cr.contractOld = :contract or cr.contractRenew = :contract')
                ->setParameter('contract', $contract)
                ->orderBy('cr.contractRenew', 'DESC')
                ->groupBy("cr.id")
                ->setMaxResults(1)
                ->getQuery()
                ->getOneOrNullResult();

    }


    public function findBySingle($id)
    {
        return $this->createQueryBuilder('cr')
                ->leftJoin('cr.contractOld', 'c')
                ->where('cr.contractRenew =:parent')
                ->setParameter('parent', $id)
                ->orderBy('cr.id', 'ASC')
                ->groupBy("cr.id")
                ->getQuery()
                ->getResult();
    }

}

I don't know if it is well explained and most importantly if it is well developed on my part. If you can help improve it or tell me if it is well done or not it would be wonderful.

\$\endgroup\$
  • 1
    \$\begingroup\$ Please do not update the code in your question to incorporate feedback from answers, doing so goes against the Question + Answer style of Code Review. This is not a forum where you should keep the most updated version in your question. Please see what you may and may not do after receiving answers. \$\endgroup\$ – Mast Jul 11 '18 at 20:55
0
\$\begingroup\$

The readability of your code improves when you create a method that returns a value or throw an Exception as soon as possible. Something like this:

// added the nullable return type (PHP 7.1+)
function getRenewals(ContractRenewal $renewalFindParent) : ?ContractRenewal
{
    if ($renewalFindParent) {
        if ($renewalFindParent->getParent()) {

            return $this->findByParent($renewalFindParent->getParent());

        }

        $newParent = $this->findOneBy(["contractOld" => $renewalFindParent->getcontractRenew()]);

        if ($newParent) {
            return $this->findByParent($newParent->getParent());
        }

        return $this->findBySingle($contract->getId());

    }

    return null;
}

This code doesn't work right now ($contract doesn't exist, findByParent and findBySingle return multiple entries), but I hope you get an idea how to improve your code. I think adding a return types is a good idea because it makes breaking the 'single responsibility principle' a bit harder (but not impossible).

I don't see any loops of self-referencing methods in your, so I can't see why your application creates many queries to the database. Please share some more code

\$\endgroup\$
  • \$\begingroup\$ Thanks really, it's a great help, as you told me I've expanded the information by putting the view and method of the controller, so that you understand everything I'm doing. I hope this is all right. \$\endgroup\$ – jcarlosweb Jul 10 '18 at 7:12
  • 1
    \$\begingroup\$ @jcarlosweb Usually it isn't, since code edits may invalidate existing answers. However, in this case the answer arrived too early and should've been written only after the context you just added was there. For future reference, see what you may and may not do after receiving answers. But for now, it's ok. \$\endgroup\$ – Mast Jul 10 '18 at 7:57
  • \$\begingroup\$ @Mast He only added some information, not changed it. \$\endgroup\$ – Stephan Vierkant Jul 10 '18 at 8:57

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.