0
\$\begingroup\$

I have the following function that returns StaffQualifications where either the qualification is expiring or the qualification type is expiring, but this section doesn't seem to sit well with me as it splits up the SQL string... Should this be two separate functions perhaps even though 95% of the query is the same?

    if ($qualification instanceof Qualification) {
        $sql .= "AND q.id = :qualificationId ";
    } elseif ($qualificationType instanceof QualificationType) {
        $sql .= "AND q.qualification_type_id = :qualificationTypeId ";
    }

The above doesn't quite feel right, but not sure how to better it.

/**
 * @return StaffQualification[]
 */
public function getStaffQualificationsExpiringOnQualificationOrQualificationType(?Qualification $qualification, ?QualificationType $qualificationType): array
{
    if ($qualification === null && $qualificationType === null) {
        throw new \Exception('Qualification or QualificationType must not be null');
    }
    if ($qualification instanceof Qualification && $qualificationType instanceof QualificationType) {
        throw new \Exception('Only one of Qualification or QualificationType can be present');
    }

    $sql = "
    SELECT *
    FROM staff_qualification AS sq ";
// Complicated SQL ensues that I've omitted from this SE question...

    if ($qualification instanceof Qualification) {
        $sql .= "AND q.id = :qualificationId ";
    } elseif ($qualificationType instanceof QualificationType) {
        $sql .= "AND q.qualification_type_id = :qualificationTypeId ";
    }

    $sql .= "
    GROUP BY sq.id
    ORDER BY sq.end_date
    ;
    ";

    $rsm = new ResultSetMapping();
    $rsm->addEntityResult(StaffQualification::class, 'sq');
    $rsm->addJoinedEntityResult(Staff::class, 's', 'sq', 'staff');
    $rsm->addFieldResult('sq', 'id', 'id');
    $rsm->addFieldResult('s', 'staff_id', 'id');
    $query = $this->getEntityManager()->createNativeQuery($sql, $rsm);
    if ($qualification instanceof Qualification) {
        $query->setParameter('qualificationId', $qualification->getId());
    } elseif ($qualificationType instanceof QualificationType) {
        $query->setParameter('qualificationTypeId', $qualificationType->getId());
    }
    $result = $query->getResult();
    return $result;
}
\$\endgroup\$
2
  • 1
    \$\begingroup\$ We need to know what the code is intended to achieve. To help reviewers give you better answers, please add sufficient context to your question, including a title that summarises the purpose of the code. We want to know why much more than how. The more you tell us about what your code is for, the easier it will be for reviewers to help you. The title needs an edit to simply state the task, rather than your concerns about the code. \$\endgroup\$ Nov 23 at 11:57
  • 3
    \$\begingroup\$ It also appears that your code has been eviscerated - no need for that here; we prefer to review full functions or programs. \$\endgroup\$ Nov 23 at 11:57
0
\$\begingroup\$

I think the best thing to do would be to extract the logic outside of the string building and pass it in. You could do this either inline as per below or with sprintf or Heredoc.

/**
 * @param ?Qualification $qualification
 * @param ?QualificationType $qualificationType
 * @return StaffQualification[]
 * @throws Exception
 */
public function getStaffQualificationsExpiringOnQualificationOrQualificationType(
    ?Qualification $qualification,
    ?QualificationType $qualificationType
): array
{
    if ($qualification === null && $qualificationType === null) {
        throw new Exception('Qualification or QualificationType must not be null');
    }
    if ($qualification instanceof Qualification && $qualificationType instanceof QualificationType) {
        throw new Exception('Only one of Qualification or QualificationType can be present');
    }

    if ($qualification instanceof Qualification) {
        $filterSql = "q.id = :qualificationId ";
    } elseif ($qualificationType instanceof QualificationType) {
        $filterSql = "q.qualification_type_id = :qualificationTypeId ";
    }
    $sql = "
    SELECT *
    FROM staff_qualification AS sq
    WHERE 1=1
    AND $filterSql
    GROUP BY sq.id
    ORDER BY sq.end_date
    ;
    ";

    $rsm = new ResultSetMapping();
    $rsm->addEntityResult(StaffQualification::class, 'sq');
    $rsm->addJoinedEntityResult(Staff::class, 's', 'sq', 'staff');
    $rsm->addFieldResult('sq', 'id', 'id');
    $rsm->addFieldResult('s', 'staff_id', 'id');
    $query = $this->getEntityManager()->createNativeQuery($sql, $rsm);
    if ($qualification instanceof Qualification) {
        $query->setParameter('qualificationId', $qualification->getId());
    } elseif ($qualificationType instanceof QualificationType) {
        $query->setParameter('qualificationTypeId', $qualificationType->getId());
    }
    $result = $query->getResult();
    return $result;
}
\$\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.