Model database for contract renewal

Question

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.

Answer 1

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