3
\$\begingroup\$

First of all, I apologize for the length of this but I'm trying to show the data I'm working with and that I attempted to figure it out on my own.

I'm building a social site for musicians. I would like to take the whole list of songs from the database and rank each one based on its relevancy to the logged in user. One reason for this is that I'd like there to always be 10 songs recommended, even if the user signed up 45 seconds ago.

The factors I'm using are:

  • The band members of songs (all would be members of the site, might have all quit the song)
  • The logged in user's member connections (may be none)
  • The most recent update in the song (will at least be the day the song was "created")
  • The (sub)genre of the song (will always be set)
  • The (sub)genre of the user (will always be set)

The (sub)genres belong to more general genres, so I figure I can either weight a song higher if the logged in user and the song's (sub)genre are the same, or a little less higher if they at least belong to the same general genre.

I'm not sure if I should be aiming for an Apriori or Bayesian algorithm... It seems to be somewhere in between. I have a couple of lines borrowed from explanations of the Hacker News algorithm that I've tweaked. I'm not quite sure how to put the final results together to get my ranking.

Here's the code I've written so far (for all intents and purposes, a studio is a song). I know queries in loops like this are never a good idea, but I've done it this way because there's some memcaching schemes I use and this will probably also be batched:

function studio_relevance($user_id, $user_genre) {
    global $cxn; //MySQL connection

    $query = "SELECT * FROM studio WHERE project_status='active'";
    $result = mysqli_query($cxn, $query) or die($query.': '.mysqli_error($cxn));

    $data = array();

    while ($studio = mysqli_fetch_object($result)) {
        //Find similarities in social connections and band
        $query_b = "SELECT * FROM band WHERE studio_id=".$studio->studio_id;
        $result_b = mysqli_query($cxn, $query_b) or die($query_b.': '.mysqli_error($cxn));

        $the_band = array();
        while ($people = mysqli_fetch_array($result_b, MYSQLI_ASSOC)) {
            $the_band[] = $people['user_id'];
        }

        $studio_band_count = count($the_band);

        $query = "SELECT * FROM idols WHERE friend_id=".$user_id;
        $result_b = mysqli_query($cxn, $query_b) or die($query_b.': '.mysqli_error($cxn));

        $idol = array();
        while ($people = mysqli_fetch_array($result_b, MYSQLI_ASSOC)) {
            $idol[] = $people['artist_id'];
        }

        $same_band = array_intersect($the_band, $idol);
        $same_band_count = count($same_band);

        ########
        $similar_band = $same_band_count / $studio_band_count;
        ########


        //Find the most recent activity
        $query_b = "SELECT * FROM studio_feed WHERE studio_id=".$studio->studio_id." ORDER BY feed_id DESC LIMIT 1";
        $result_b = mysqli_query($cxn, $query_b) or die($query_b.': '.mysqli_error($cxn));
        $tab = mysqli_fetch_object($result_b);

        $time_diff = strtotime('now') - strtotime($tab->timestamp);
        $hours = $time_diff / 3600;

        ########
        $last_activity = pow(($hours+2), 1.8);
        ########


        //Compare genres
        $genre_weight = 1;

        if ($studio->songGenre == $user_genre) {
            $genre_weight = 3;
        } else {
            $query_b = "SELECT * FROM genres";
            $result_b = mysqli_query($cxn, $query_b) or die($query_b.': '.mysqli_error($cxn));

            $genres = array();
            $user_genre_cat = 0;

            while ($genre = mysqli_fetch_object($result_b)) {
                $genres[$genre->genre_cat][] = $genre->genre_id;

                if ($genre->genre_id == $user_genre) {
                    $user_genre_cat = $genre->genre_cat;
                }
            } 

            if (in_array($studio->songGenre, $genres[$celeb_cat])) {
                $genre_weight = 2;
            }
        }

        //Find final result
        //$final = $similar_band + $last_activity + $genre_weight;
        //$hours / pow(($similar_band+2), $genre_weight);
    }

    return $data;
}

Any suggestions would be greatly appreciated! Am I on the right track or going about this all wrong?

\$\endgroup\$
8
  • \$\begingroup\$ What is $cxn? \$\endgroup\$ Commented Feb 1, 2012 at 16:04
  • \$\begingroup\$ It's the MySQL database connection. \$\endgroup\$
    – grandcameo
    Commented Feb 1, 2012 at 16:22
  • \$\begingroup\$ I'm assuming it was created with mysqli_connect() ? \$\endgroup\$ Commented Feb 1, 2012 at 16:26
  • \$\begingroup\$ Correct! It was created in a master config file and all queries not returned by memcache use it. \$\endgroup\$
    – grandcameo
    Commented Feb 1, 2012 at 16:29
  • \$\begingroup\$ Long functions are usually a code smell. They indicate you're trying to do too much in one go where the problem you're trying to solve can be further subdivided (and therefore bits of it reused, the bits in isolation understood better, etc). A good rule of thumb is you should be able to fit an entire function on screen in your IDE without scrolling. \$\endgroup\$
    – GordonM
    Commented Feb 2, 2012 at 8:37

1 Answer 1

2
\$\begingroup\$

Your function is way too long. This is a lot going on in one function and very likely has some good candidates for smaller, single-purpose functions to be abstracted out of it. This is very likely the first thing that I would work on, it would make the flow of your function a little better and would likely make your function a lot easier to read.

I also noticed that you are highly susceptible to SQL injection attacks. You should be, at the very least, escaping the user input before you put it into the database and would be even better if you decided to use prepared statements. I put in a code example below that details how you would go about changing one of your queries to use prepared statements.


We'll be swapping over to using prepared statements so you need to make sure that you have the mysqlnd extension properly installed.

function getPreparedStatement($sql, array $parameters = array()) {
    global $cxn;

    if (!is_string($sql)) {
        trigger_error('The SQL query passed must be in the form of a string.', E_USER_WARNING);
        return false;
    }

    $stmt = mysqli_prepare($cxn, $sql);
    if (!$stmt) {
        trigger_error(mysqli_error($cxn), E_USER_WARNING);
        return false;
    }

    // we are subtracting 1 from this because the parameters passed should include the 
    // 'xxx' string denoting data type
    $numPassedParameters = count($parameters) - 1;
    $stmtParamCount = mysqli_stmt_param_count($stmt);

    if ($stmtParamCount !== $numPassedParameters) {
        trigger_error('The number of parameters passed must match the number of parameters in the statement.', E_USER_WARNING);
        return false;        
    }

    if ($stmtParamCount > 0) {
        $dataTypes = array_shift($parameters);
        $refArray = array();
        $refArray[] = $cxn;
        $refArray[] = $datatypes;
        // we are doing this because mysqli_stmt_bind_param is expecting a reference
        foreach ($parameters as $key => $value) {
            $refArray[] =& $parameters[$key];
        }

        return call_user_func_array('mysqli_stmt_bind_param', $refArray));
    } else {
        return $stmt;
    }

}

So, as an example of using this new function to get a prepared statement:

$query_b = "SELECT user_id FROM band WHERE studio_id=?";
$query_b_parameters = array('i', $studio->studio_id);  // assume this int change 'i' to 's' if a string
$stmt_b = getPreparedStatement($query_b, $query_b_parameters);
$result_b = mysqli_stmt_execute($cxn, $stmt_b);
if (!$result_b) {
    trigger_error(mysqli_error($cxn));
    return false;
}

$the_band = array();
while ($people = mysqli_fetch_array($result_b, MYSQLI_ASSOC)) {
    $the_band[] = $people['user_id'];
}

So, now you're using prepared statements for this query and it is not as susceptible to SQL injection. Also, notice that I explicitly defined that I only want user_id from the query and not all the things. You should be able to figure out from there how to replace the other SQL queries with prepared statements.


Some other things that I've noticed:

  • Under $query you have $result_b again and are requerying $query_b. Likely you meant to just send $query
  • Not necessarily an error but the * in SQL statements. If you aren't really getting all the data try to explicitly state what you want.
  • Try to find a way to abstract getting the results from a query in the form of an array or an object into a couple functions that take a mysqli_stmt and returns the appropriate data type for the result.
  • Try to abstract out the business logic of comparing genres and finding the most recent activity into different functions so that your main function here simply controls the order in which queries get executed and then passes the appropriate queries off to functions that get the right data.

There's some things that could be done to make the function more maintainable and duplication but I think the priority right now is swapping over to prepared statements and getting rid of your SQL Injection vulnerability.

\$\endgroup\$
3
  • \$\begingroup\$ Thank you for your help. I really appreciate it. I should probably just point out first that I do have functions for a lot of those queries, I just replaced the functions with some of the actual code to show what was going on. I also am working on data that has already been cleaned with trim, strip_tags, and mysqli_escape_string, or that is coming directly from the database. I posted this here hoping to get some help more on getting to the final, weighted result rather than the actual code itself. I was a bit rudely told my post was too long on StackOverflow and to come over here. \$\endgroup\$
    – grandcameo
    Commented Feb 1, 2012 at 18:12
  • \$\begingroup\$ I will accept it, but I need to state it does not address my original post. I was looking for help with algorithms. My code was completely pseudocode. I clearly stated that my example was showing the data I was working with. I had merged the contents of several functions for demonstration. Luckily, I figured out a solution for recommendation ranking elsewhere. \$\endgroup\$
    – grandcameo
    Commented Mar 7, 2012 at 14:56
  • \$\begingroup\$ @grandcameo Ultimately a code review is just that...a review of the code presented. Any specific performance issues are completely conceptual. What you posted is not psuedocode, it looks much more like a full-blown implementation. That being said, if you feel my review didn't adequately answer your question then please do not accept it :) \$\endgroup\$ Commented Mar 9, 2012 at 4:51

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.