1
\$\begingroup\$

I have upgraded my code. In the old code I had two functions: display_maker_success() and display_maker_fail():

function display_maker_success($link, $userid){
    $status="closed";
    $result="completed";

    $html = '';
    $sql = "select start, name from wuuk where tasker_id ='$userid' and status ='$status' and result ='$result' order by id desc LIMIT 6;";

    $result = mysql_query($sql, $link);
    $isempty=mysql_num_rows($result);
    If ($isempty ==0) {
        $html .= "No Record";
    } else {
        $html .= "<table border=1>";
        $html .= "<tr><th>Date & Time</th><th>Name</th><th>Status</th></tr>";
        while ($row = mysql_fetch_array($result, MYSQL_NUM)) {
            $html .= "<tr><td>$row[0]</td><td>$row[1]</td><td>Completed</td></tr>";
        };
        $html .= "</table>";
    };
    echo $html;
};

function display_maker_fail ($link, $userid) {
    $status="closed";
    $result="fail";

    $sql = "select start, name from wuuk where tasker_id ='$userid' and status ='$status' and result ='$result' order by id desc LIMIT 1;";
    $result = mysql_query($sql, $link);
    $isempty=mysql_num_rows($result);
    If($isempty ==0){
        $html .= "No Record";
    } else {
        $html .= "<table border=1>";
        $html .= "<tr><th>Date & Time</th><th>Name</th><th>Status</th></tr>";
        while ($row = mysql_fetch_array($result, MYSQL_NUM)) {
            $html .= "<tr><td>$row[0]</td><td>$row[1]</td><td>fail</td></tr>";
        };
        $html .= "</table>";
    };
    echo $html;
};

But I realised I can combine those two functions into one display_maker_stat() by putting more arguments into the function. I like it very much!

function display_maker_stat($link, $userid, $reuslt = 'fail', $limit)
{
    $status = "closed";
    $html = '';
    $sql = "select start, name from wuuk where tasker_id ='$userid' and status ='$status' and result ='$result' order by id desc LIMIT 1;";
    $query = mysql_query($sql, $link);
    if (mysql_num_rows($query) != 0) {
        $html .= "<table border=1>";
        $html .= "<tr><th>Date & Time</th><th>Name</th><th>Status</th></tr>";
        while ($row = mysql_fetch_array($query, MYSQL_NUM)) {
            $html.= "<tr><td>$row[0]</td><td>$row[1]</td><td>$result</td></tr>";
        }
        $html.= "</table>";

    }
    else {
        $html.= "No Record";
    }
    echo $html;
}

Is there a better way to do this? I want more code reuse.

\$\endgroup\$
1
\$\begingroup\$

I used to do this sort of "optimisation" 20 years ago. I don't anymore. Clarity of code is more important. I would rather have fewer parameters and properly named functions. This way, you can just read the code without worrying about how the function works.

If you do what you are proposing, then to understand the code, you have to look at the what the function is doing every time it is used !!!

EDIT An example of what I have just finished doing at work. The original procedure was called Launch_Client_Form with a whole bunch of parameters. It had four functions and the parameters of all 4 functions had to be passed each time. I created 4 procedures

  • View_Client
  • Create_Client
  • Edit_Client
  • Find_Client

Each one has only the parameters it requires. Internally they all still call Launch_Client_Form. Now, when you encounter Edit_Client in the code, it does not require a comment and you know what it is going to do.

\$\endgroup\$
  • \$\begingroup\$ thx :) Is this OOP? Because I don't know OOP. can u give me more code that inside launch_client_form procedure? \$\endgroup\$ – n00dle Oct 24 '15 at 4:27
0
\$\begingroup\$

Generally, I would combine the two ways:

function display_maker_success($link, $userid){
    display_maker_stat($link, $userid, "completed");
}

function display_maker_fail ($link, $userid) {
    display_maker_stat($link, $userid, "completed");
}

function display_maker_stat($link, $userid, $reuslt = 'fail'){
    [...]
}

Now it's still easy to use these functions, but you get rid of the duplication, which increases maintainability.

Is there a better way to do this?

In this case I would create more functions. This should increase the readability and reusability of your code:

selectWuuks($link, $userId, $status, $result) {
    // select the wuuk and return it, including the result column.
    // if you want to, you can also create a wuuk class for this, so you
    // get rid of arrays, whose structure is often hard to 
    // remember or use for other users of your functions.
}

createWuukTable($wuuks) {
    $html = "<table border=1>";
    $html .= "<tr><th>Date & Time</th><th>Name</th><th>Status</th></tr>";
    foreach ($wuuks as $wuuk) {
        $html .= "<tr><td>$wuuk['start']</td><td>$wuuk['name']</td><td>$wuuk['status']</td></tr>";
    }
    $html .= "</table>";
    return $html;
}

function display_maker_success($link, $userid){
    $wuuks = selectWuuks($link, $userid, "completed", "closed");
    echo createWuukTable($wuuks);
}
[...]

Misc

  • Security: You are open to SQL injection. Always use prepared statements (with mysqli or PDO).
  • mysql_* is deprecated (and has been for quite a while), you should not use it anymore.
  • You are not consistent with your spacing, which hurts readability.
  • all SQL keywords should be all uppercase to increase readability.
  • don't reuse variables for completely different things (eg result).
  • You don't really need a semicolon after a curly bracket.
  • use camelcase for your variable names (eg isEmpty).
  • Is a wuuk an actual thing, or did you abbreviate? If it's the second, I would write it out.
  • reuslt should probably be result.
  • I would rename $link to $db for clarity.
\$\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.