6
\$\begingroup\$

I currently have these two methods. One method returns all records, while the other one returns only a specific record.

function GetLeaveRequestDetailsAll($leaveCurrentStatus){

    global $connPDO;

    $sql = "select *, emp.Employee_Name, emp.Designation, emp.Emp_Dept_Category, lt.Leave_Name
            from tbl_emp_leave_details eld
            Inner join tbl_leave_types lt
            ON lt.leave_id = eld.leave_type
            Inner Join employee emp
            on emp.PK = eld.emp_id
            Inner JOIN dept_category dc
            ON emp.Emp_Dept_Category = dc.PK
            where eld.leave_current_status_id = :leave_current_status_id";

    $binds = array(
        ":leave_current_status_id"   =>  $leaveCurrentStatus
    );

    $statementEntryCount = $connPDO->prepare($sql);

    $statementEntryCount->execute($binds);
    $queryData = $statementEntryCount->fetchAll(PDO::FETCH_ASSOC);
    echo json_encode($queryData);
}


function GetLeaveRequestDetailsById($leaveCurrentStatus, $leaveDetailId){

    global $connPDO;

    $sql = "select *, emp.Employee_Name, emp.Designation, emp.Emp_Dept_Category, lt.Leave_Name
            from tbl_emp_leave_details eld
            Inner join tbl_leave_types lt
            ON lt.leave_id = eld.leave_type
            Inner Join employee emp
            on emp.PK = eld.emp_id
            Inner JOIN dept_category dc
            ON emp.Emp_Dept_Category = dc.PK
            where eld.leave_current_status_id = :leave_current_status_id
            And eld.leave_detail_id = :leave_detail_id";

    $binds = array(
        ":leave_current_status_id"   =>  $leaveCurrentStatus,
        ":leave_detail_id"   =>  $leaveDetailId
    );

    $statementEntryCount = $connPDO->prepare($sql);

    $statementEntryCount->execute($binds);
    $queryData = $statementEntryCount->fetchAll(PDO::FETCH_ASSOC);
    echo json_encode($queryData);
}

As you can see, both are doing the same work with a small difference, which is leave_detail_id. How can I convert these two functions into a single parameterized method to handle both queries?

\$\endgroup\$
6
\$\begingroup\$

If I assume that leave_detail_id contains an auto-increment value, starting at 1, you can do:

function queryDatabase($query, $bindings = [])
{
    global $connPDO;
    // prepare statement 
    $statement = $connPDO->prepare($query);
    // execute statement    
    $statement->execute($bindings);
    // return result in associative array
    return $statement->fetchAll(PDO::FETCH_ASSOC);
}

function getLeaveRequestDetails($leaveCurrentStatus, $leaveDetailId = 0)
{
    // build query
    $query = "SELECT *, 
                     emp.Employee_Name, 
                     emp.Designation, 
                     emp.Emp_Dept_Category, 
                     lt.Leave_Name
              FROM tbl_emp_leave_details eld
              INNER JOIN tbl_leave_types lt ON lt.leave_id = eld.leave_type
              INNER JOIN employee emp ON emp.PK = eld.emp_id
              INNER JOIN dept_category dc ON emp.Emp_Dept_Category = dc.PK
              WHERE eld.leave_current_status_id = :leave_current_status_id";
    // define bindings
    $bindings = [":leave_current_status_id" => $leaveCurrentStatus];
    // should details id be use? 
    if ($leaveDetailId > 0) {
          $query .= " AND eld.leave_detail_id = :leave_detail_id";
          $bindings[":leave_detail_id"] = $leaveDetailId;
    }
    // query database and return result
    return queryDatabase($query, $bindings);
}

I used a default function argument value to set $leaveDetailId to zero, if it is absent, when the function is called. If it does get a value bigger than zero the condition is added to the query and the value is bound.

I also removed the actual database querying from this function and put it in a separate function. The reason is that this code can probably be used more often in your scripts. Instead of repeating it every time you can now call queryDatabase().

Finally I removed the echo json_encode(); from the function. The function name starts with "get" and that almost always implies something will be returned. Also, echoing JSON would restrict this function to that task alone. Now you can still use it to echo JSON, but you can also use it to do something else with the result. A call, returning JSON, would look something like this:

echo json_encode(getLeaveRequestDetails("fallen",15));

As you can see I've now split your code into more parts than you had before. That's not a bad thing. Now each part performs a single function: Querying the database, getting details of a leave, and echoing JSON. This makes your code easier to read.

In summary:

\$\endgroup\$
  • \$\begingroup\$ This is almost exactly an answer I was writing. I would only add a warning about using global. and offer to pass the connection as a parameter. \$\endgroup\$ – Your Common Sense Jun 28 at 7:36
  • \$\begingroup\$ Awesome way to short the code, it is helpful in the context of the question. But as @YourCommonSense suggest we cannot use insert with the specific method. \$\endgroup\$ – Muhammad Faizan Khan Jun 28 at 10:23
  • \$\begingroup\$ This is throwing error "$bindings = []" \$\endgroup\$ – Muhammad Faizan Khan Jun 28 at 10:59
  • 1
    \$\begingroup\$ Which version of PHP are you using? Ref.: "As of PHP 5.4 you can also use the short array syntax, which replaces array() with [].". So you could replace it with $bindings = array(). \$\endgroup\$ – KIKO Software Jun 28 at 11:58
  • 1
    \$\begingroup\$ @NicHartley it is quite possible with PHP, I've got an example: phpdelusions.net/pdo_examples/dynamical_where#static \$\endgroup\$ – Your Common Sense Jul 1 at 8:16
4
\$\begingroup\$

I was about to write an answer almost literally the same as written by KIKO Software (even with the same hyperlinks), so I would only elaborate on it (and somewhat make an answer for the generic question from the question title).

Both queryDatabase() and getResult() functions from the other two answers are too specific. They cut off the PDO's great ability to return the database result in dozens different formats. So instead of returning just a single format, I would make it rather a generic PDO query function that returns a PDOStatement instance, from which the data could be extracted in any format supported by PDO, so it can be used not only with this particular query but with any query you can think of

function pdo($pdo, $sql, $args = NULL)
{
    if (!$args)
    {
         return $pdo->query($sql);
    }
    $stmt = $pdo->prepare($sql);
    $stmt->execute($args);
    return $stmt;
}

Examples:

$count = pdo($pdo, "SELECT count(*) FROM users")->fetchColumn();
$user = pdo($pdo, "SELECT * FROM users")->fetch(PDO::FETCH_OBJ);
$deleted = pdo($pdo, "DELETE FROM users WHERE id=?", [$id])->rowCount();
$indexed_by_id = pdo($pdo, "SELECT id, name FROM users")
     ->fetchAll(PDO::FETCH_KEY_PAIR);

and so on. And of course it would work for your function as well:

return pdo($query, $bindings)->fetchAll(PDO::FETCH_ASSOC);

Another issue is global. Using global keyword is considered rather bad practice and could be justified for only most primitive code. but if you want your code solid and maintainable, always pass a database connection as a function parameter into every function that needs a database interaction. It's little verbose, but will pay back in the future.

\$\endgroup\$
  • \$\begingroup\$ I agree with your assessment that queryDatabase() is too specific. It makes more sense to do it this way. I'll leave my answer at it is, so not to spoil your addition. \$\endgroup\$ – KIKO Software Jun 28 at 8:26
  • \$\begingroup\$ what "this" represent in the pdo function? and you didn't use $pdo in the function too? \$\endgroup\$ – Muhammad Faizan Khan Jun 28 at 10:19
  • \$\begingroup\$ It was a typo, fixed now \$\endgroup\$ – Your Common Sense Jun 28 at 10:25
2
\$\begingroup\$

The code below reduces the duplication you have.

The SQL statement is the first thing that is duplicated, with the exception of the where clause. this means we can extract the building of the query to a function that accepts the where clause and returns our query.

The result parsing is similar, accepting two parameters instead of one. Rather than just copy the code though, we can leave the echo and json_encode in the outer function. This means we can reuse the getSelectStatement and getResult functions elsewhere, where we might not need to echo straight away. We can keep your original functions GetLeaveRequestDetailsAll and GetLeaveRequestDetailsById for the same reason.

Finally, the single parameterized function you wanted, GetLeaveRequestDetails. This has a second parameter that defaults to null - you don't have to specify it when you call the function. In practice I would use your two original functions as I believe it's clearer what's going on to people reading the top level of your code, but that's up to you.

As an aside, I'd take a look at what fields you actually need returned from your query. Using * is discouraged, particularly if you are echoing the result, as any new fields you add in the future will be printed as well. If the fields are secret, you may not want this.

function GetLeaveRequestDetails($leaveCurrentStatus, $leaveDetailId = null) {

    if ($leaveDetailId) {
        $result = GetLeaveRequestDetailsById($leaveCurrentStatus, $leaveDetailId);
    } else {
        $result = GetLeaveRequestDetailsAll($leaveCurrentStatus);
    }

    echo json_encode($result);
}

function GetLeaveRequestDetailsAll($leaveCurrentStatus) {

    $sql = getSelectStatement(
        "where eld.leave_current_status_id = :leave_current_status_id"
    );

    $binds = array(
        ":leave_current_status_id"   =>  $leaveCurrentStatus
    );

    return getResult($sql, $binds);
}


function GetLeaveRequestDetailsById($leaveCurrentStatus, $leaveDetailId) {

    $sql = getSelectStatement(
        "where eld.leave_current_status_id = :leave_current_status_id 
         And eld.leave_detail_id = :leave_detail_id"
    );

    $binds = array(
        ":leave_current_status_id"   =>  $leaveCurrentStatus,
        ":leave_detail_id"   =>  $leaveDetailId
    );

    return getResult($sql, $binds);
}

/** 
 * Both functions use the same query with a different WHERE clause, 
 * so extract that to a function
 */
function getSelectStatement($whereStatement) {

    return "select *, emp.Employee_Name, emp.Designation, 
            emp.Emp_Dept_Category, lt.Leave_Name
            from tbl_emp_leave_details eld
            Inner join tbl_leave_types lt
            ON lt.leave_id = eld.leave_type
            Inner Join employee emp
            on emp.PK = eld.emp_id
            Inner JOIN dept_category dc
            ON emp.Emp_Dept_Category = dc.PK" . $whereStatement
}

/**
 * Both functions parse the query result in the same way, so extract that to a function
 */
function getResult($sql, $binds) {

    global $connPDO;

    $statementEntryCount = $connPDO->prepare($sql);
    $statementEntryCount->execute($binds);
    $queryData = $statementEntryCount->fetchAll(PDO::FETCH_ASSOC);

    return $queryData;
}
```
\$\endgroup\$
  • \$\begingroup\$ Thanks buddy it is helpful but some of the other answer are much smarter \$\endgroup\$ – Muhammad Faizan Khan Jun 28 at 10:20
  • \$\begingroup\$ Please always endorse the consistent usage of ALL CAPS sql keywords for readability. \$\endgroup\$ – mickmackusa Jun 29 at 5:01
  • \$\begingroup\$ I am also not convinced that it is advisable to declare a function merely to prepend a string to another string. Perhaps less overhead would be to declare the front end of the query string as a globally available constant. \$\endgroup\$ – mickmackusa Jun 29 at 5:10

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.