php refactoring and better coding for php?

Question

i have this process that is the heart of my app, that im creating, but for some reason i feel like its the worst way to do it(instinct) , and i wanted to see if thier is something wrong with this process, and am i approaching it in a bad way! p.s. the code works fine, just refactoring problem.

the process is:

users go to homepage, they see thier latest activities, by other site memebers(home.php),

 //function to bring the latest activities from database
   $results=function getUserUpdates($_SESSION['user_id'];


while($row = mysql_fetch_array($results))

      {
//another function to format the activities in a social stream
          echo formatUpdate($row['user_note'],$row['dt'],$row['picture'],$row['username'],$row['id'],$row['reply_id'],$row['reply_name'],$row['votes_up'],$row['votes_down']);


      }

i have put the function codes in pastie.

formatUpdate function http://pastie.org/1213958

getUserUpdates function http://pastie.org/1213962

EDIT both functions are from different files they are included in home.php, formatUpdate from functions.php getUserUpdates from queries.php

Answer 1

Firstly, I think you mean:

$results = getUserUpdates($_SESSION['user_id']);

In your getUserUpdates() function there is a redundant branch:

if ($username == $_SESSION['u_name']){
    // return something
}

if ($username != $_SESSION['u_name']){
    // return something else
}

You don't need the second if statement as any code run at that point will only be run if $username != $_SESSION['u_name'].

In my opinion, it's usually better not to have different functions directly echoing HTML up the stack (such as echoVote()). It's preferred to have functions return data and have the original caller echo it. This allows the caller to perform additional data massaging if desired.

Other than that, your code is fetching data, looping through and acting on the results which is pretty much standard fare.

I think your instinct is to be a little too harsh on yourself ;) There are improvements to be made but it's certainly not the worst way to do anything.

Answer 2

First of all, it's good that you have separate functions for getting the data and for formatting the data. It's a good start toward refactoring your code. It makes it easier in the future: if you ever want to format your data differently, you can just expand your formatter.

Second, this is what coreyward meant by a lambda:

$results=function getUserUpdates($_SESSION['user_id'];

Remove the function keyword. You use function when you're defining a function. But here you're only calling one. (You defined it in queries.php.)

Third, I agree with webbiedave about the echo statements. A good way to avoid that: In the "heart" of your app, collect all the HTML into one place. Then, when you've collected everything you're going to display on the page, you can echo it all at once. This makes it a lot easier to keep track of what you're doing, and to remember the order of everything. It also makes it easier to add headers and footers, or do more formatting. Otherwise, if you have echo statements scattered around your code, it's a lot easier to let something slip that shouldn't be there.

Here's a very basic example of what I mean:

$html = '';
$results = getUserUpdates($_SESSION['user_id'];

while($row = mysql_fetch_array($results)) {
    $fields = array(
        'user_note' => $row['user_note'],
        'dt' => $row['dt'],
        'picture' => $row['picture'],
        'username' => $row['username'],
        'id' => $row['id'],
        'reply_id' => $row['reply_id'],
        'reply_name' => $row['reply_name'],
        'votes_up' => $row['votes_up'],
        'votes_down' => $row['votes_down'],
    );
    $html .= formatUpdate($fields);
}
// This way you can do whatever you want to $html here.
echo $html;

Also notice that I put all the fields from $row into an array and passed it to formatUpdate(). That has two advantages:

1. It's easier to read.
2. If you ever want to change the fields that formatUpdate deals with, you don't have to worry about searching through your code to change the arguments every time you call it.