Sign up ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I am new to OOP and have written a products class. All is working fine but I am unsure which of the below version of a method within this class is best?

The first gets the variables from within the object and the second passes the variables into the class. Both work. I originally had it as the first version but things seems to be running slow and then changed it to the second.

public function getProductURLstart(){  
    $select = "SELECT l.URL, p.id FROM logins AS l
        INNER JOIN Pages AS p ON l.id = p.clientID 
        WHERE l.id = '$this->skID' AND p.productPage = 1";

    $res = mssql_query($select);
    $r = mssql_fetch_row($res);     

    $url = trim($r[0]); 
    $page_id = $r[1];

    return  $url .'/index.aspx?pageID='. $page_id . '&prodID=$this->prodID';
}

OR

static function getProductURLstart($skID, $prodId){    
    $select = "SELECT l.URL, p.id FROM logins AS l
        INNER JOIN Pages AS p ON l.id = p.clientID 
        WHERE l.id = '$skID' AND p.productPage = 1";

    $res = mssql_query($select);
    $r = mssql_fetch_row($res);     

    $url = trim($r[0]); 
    $page_id = $r[1];

    return  $url .'/index.aspx?pageID='. $page_id . '&prodID=$prodId';
}
share|improve this question

1 Answer 1

Which is the better should not depend on mainly speed. I think you should post more code (at least the name of class and its responsibilities or the list of its functions).

Some other notes:

  1. It's really hard to read code which contains one character long variable names:

    $r = mssql_fetch_row($res);
    

    You have to decode them. I'd rename it to $row. The same true for the SQL queries: logins AS l. I would let it be logins.

  2. In the second version there are three variable naming convention: $skID, $prodId, $page_id. Try to be consistent and use just one: they should be $skId, $prodId, $pageId or $skID, $prodID, $pageID or $sk_id, $prod_id, $page_id. (I think the first, the camelCase version is the most readable.)

  3. There is no error handling in the code. What happens when the query returns empty results?

  4. Probably the code is vulnerable to SQL injections: http://en.wikipedia.org/wiki/SQL_injection

  5. Try accessing database attributes with keys not numeric indexes.

    $url = trim($r[0]); 
    $page_id = $r[1];
    

    The following would be more readable:

    $url = trim($r["url"]); 
    $page_id = $r["id"];
    
share|improve this answer
    
wow, thanks for you advice, however I was looking for help with OOP and Im still none the wiser... – leanne Nov 30 '11 at 10:27
    
"I think you should post more code (at least the name of class and its responsibilities or the list of its functions)." :-) – palacsint Nov 30 '11 at 11:02

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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