Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I am working on a little browsergame project written in PHP and using PostgreSQL as DBMS. Now I'm not really lucky with the process started after a userlogin was succesful.


Some info:

  • there are 3 different kinds of properties a game character can have:

    • attributes
    • skills
    • talents
  • each of these properties is a table in my database
  • each of these properties is related to the character table in an extra table
  • after the login was successful I want to store both general information about these properties and the character-related values of them in the session (the first in 'game' and the second in 'user')

How I currently get the data:

[...]
$this->getIngameInfo();

//one account can have up to 4 characters
//each of the characters can have different values          
foreach($_SESSION['user']['character'] as $key => $data){
    $_SESSION['user']['character'][$key]['attribute'] = $this->getAttributes($data['id']);
    $_SESSION['user']['character'][$key]['skill'] = $this->getSkills($data['id']);
    $_SESSION['user']['character'][$key]['talent'] = $this->getTalents($data['id']);
}
[...]

private function getIngameInfo(){
    $sql = "SELECT id,
    name,
    tag,
    description
    FROM attribute";
    if($this->db->query($sql, array())){
        while($row = $this->db->fetchAssoc()){
            $_SESSION['game']['attribute'][] = $row;
        }
    }

    $sql = "SELECT id,
    name,
    tag,
    description
    FROM skill";
    if($this->db->query($sql, array())){
        while($row = $this->db->fetchAssoc()){
            $_SESSION['game']['skill'][] = $row;
        }
    }

    $sql = "SELECT id,
    name,
    description
    FROM talent";
    if($this->db->query($sql, array())){
        while($row = $this->db->fetchAssoc()){
            $_SESSION['game']['talent'][] = $row;
        }
    }
}

private function getAttributes($charid){
    $sql = "
        SELECT attributeid,
        value
        FROM character_attribute
        WHERE characterid = $1
  ORDER BY attributeid ASC
        ";

    $attributes = array();

    if($this->db->query($sql, array($charid))){
        while($row = $this->db->fetchAssoc()){
            $attributes[] = $row;
        }
    }
    return $attributes;
}

private function getSkills($charid){
    $sql = "
        SELECT skillid,
        value
        FROM character_skill
        WHERE characterid = $1
  ORDER BY skillid ASC
        ";

    $skills = array();

    if($this->db->query($sql, array($charid))){
        while($row = $this->db->fetchAssoc()){
            $skills[] = $row;
        }
    }
    return $skills;
}

private function getTalents($charid){
    $sql = "
        SELECT talentid,
        value
        FROM character_talent
        WHERE characterid = $1
  ORDER BY talentid ASC
        ";

    $talents = array();

    if($this->db->query($sql, array($charid))){
        while($row = $this->db->fetchAssoc()){
            $talents[] = $row;
        }
    }
    return $talents;
}

I now wonder how I could merge these quite similiar queries, because I'll need to fetch more information after that and I don't like firing so much queries in one process.

I thought about using prepared statements (I use a self-written pgsql-PDO-class), but I am not calling the same table multiple times (and table 'talent' does not have exactly the same columns as the other both).
I also mentioned creating one or two stored procedures which return all the needed data. But in this case I would not know how to assign such a bunch of data to the different named sessionarrays.

So I would like to know:

  • how to reduce the queries
  • simplify the php-code/improve performance of the script

If there are other things I could improve in the code (oop-related or general), please let me know.
Thanks.


edit:
Sorry that I was very focused on the particular problem I've seen and didn't mention/point out some more facts. The methods shown belong to a loginmodel and are called only one time. I used the sessionarray because the properties of a character should be shown in different ways (which would lead to caching) and used for calculations in different ways. As I don't like firing queries against the db to calculate with values that maybe did not change I didn't see a real alternative to sessions. Think about that:

  • fetch character properties once after login
  • depending on user's interactions, show (cached if not changed) or calculate (? if not changed) with these properties
  • depending on user's interactions, change these properties, update db and update session

Maybe someone has an idea for the ?.

Related to the very useful answers I will change my code to match the named principles.
TODO:

  • encapsulate sessiondata in another model
  • use prepared queries for "getAttributes", "getSkills" and "getTalents"
  • & sum them to one method
  • & move it to another model, as it will be not only needed when logging in, but when chars interact with other chars (wasn't away of)
share|improve this question
    
I'm too lazy to write out a full answer, and this was hinted at by the answers below, but a little note: storing the data in a session has a major problem that wasn't made explicit. What if the data changes while it's in a session? Suddenly you've stored stale data. This gets even more complicated if it's possible for someone other than the session holder to alter the data. There's no clean way to 'invalidate' session data across all sessions. $_SESSION is not a magical cache. Using it as a cache is typically a sign of premature optimization (what I think it mostly is here) or flawed design –  Corbin Jan 5 '13 at 7:29
add comment

2 Answers

up vote 2 down vote accepted

I agree with MECU about not storing everything in the session. Caching is probably the best way to go. Sessions are typically used for continuity between page loads. Meaning you can store information like the character ID or login status, but the rest should be done differently. So, even though I'm about to start explaining how to better do what you are trying to accomplish with your sessions, I hope you will apply it to whatever new method you come up with.

Speaking of sessions. Using an outside resource, such as sessions or cookies or post, is a violation of the Law of Demeter (LoD). Simply put, this law suggests that your code, either method/function or class, not know more than is necessary to accomplish its task. Right now you have your entire class tightly coupled with the session. What if, as both MECU and I suggested, you wanted to move away from using the session? Then you'd have to rewrite this entire class. The better thing to do would be to write this class in such a way as to not be dependent upon it in the first place. You could instead return these same arrays to your main application to then apply them to the session array or even a cache. You could also inject any outside parameters you needed into your method's arguments to share initial values. Always try to write your code so that it is as reusable as possible.

Now, you can more easily get the character stats by following the "Don't Repeat Yourself" (DRY) Principle. As the name implies, your code should not repeat itself. So, instead of writing out that long array pointer multiple times, you can more easily, and cleanly, create a new array and merge the two when you are done. Additionally, you can abstract the $data[ 'id' ] to its own variable as well to make this even easier.

$stats = array();
foreach( $_SESSION[ 'user' ] [ 'character' ] AS $key => $data ) {
    $id = $data[ 'id' ];

    $stats[ $key ] = array(
        'attribute' => $this->getAttributes( $id ),
        'skill'     => $this->getSkills( $id ),
        'talent'    => $this->getTalents( $id )
    );
}

//use array_merge_recursive if you want to keep original $data array
array_merge(
    $_SESSION[ 'user' ] [ 'character' ],
    $stats
);

Seen as how you use the same "id" to access the attributes, skills, and talents, it would make more sense to create a new method to get all three and return the results in an array, similar to the one above. This follows two core OOP principles, the first I already mentioned, DRY, and Single Responsibility. This new principle means that our methods should be responsible for just one thing. If our methods are responsible for more than one task, then that makes them harder to reuse and we typically end up repeating code to accomplish similar tasks, which violates the first principle again. Its a vicious cycle.

private function getStats( $id ) {
    return array(
        'attribute' => $this->getAttributes( $id ),
        'skill'     => $this->getSkills( $id ),
        'talent'    => $this->getTalents( $id )
    );
}

MECU mentioned something similar, but I think it should be elaborated. He expressed a dislike for using non-conditionals as a conditional statement. This is a good thing to be adverse to. Complex conditionals should also be avoided. Complex meaning nested parenthesis, or long lists of conditionals, or even just a long condition. I don't see any of the latter, so I'll just cover the first. Both of these types of statements tend to cause issues with legibility, thus the need for abstracting the conditional to a variable. At first glance the first statement is hard to read because the parenthesis tend to run together. The second statement is a little better, but only because I added whitespace around the parenthesis, this just happens to be my style for this very reason. The third abstracts this to avoid excessive nesting, making it even easier to read than the second and allows for potential expansion should you want to use the $result later.

//a complex conditional
if($this->db->query($sql, array())){
//a complex conditional using whitespace
if( $this->db->query( $sql, array() ) ) {

//compared to...
$result = $this->db->query( $sql, array() );
if( $result ) {

I demonstrated DRY a couple of times above, so I'll leave this one to you. Your getIngameInfo() shows a more standard violation of DRY. It queries the database three times in a very similar method. The only thing that really changes is the SQL used and the portion of the session "game" array. I would suggest creating a new method to accomplish this for you. In fact, that new method can probably be reused for the getAttributes(), getSkills(), and getTalents() methods as well. I would use those later 3 methods as a template and use the returned array to populate your session.

Hope this helps!

share|improve this answer
    
That helps a lot ;) thanks for pointing out the principles, I will keep them in mind. Instead of copying values like $id = $data[ 'id' ] I would prefere using references - do you see problems here? –  32bitfloat Jan 5 '13 at 0:37
    
@32bitfloat: What do you mean by references? These are references and they don't seem to fit with your question nor do I even see how using them would help. –  mseancole Jan 7 '13 at 15:13
add comment

Here's how I would write this:

[...]
$this->getIngameInfo();

//one account can have up to 4 characters
//each of the characters can have different values          
foreach($_SESSION['user']['character'] as $key => $data){
    $_SESSION['user']['character'][$key]['attribute'] = $this->getAttributes($data['id']);
    $_SESSION['user']['character'][$key]['skill'] = $this->getSkills($data['id']);
    $_SESSION['user']['character'][$key]['talent'] = $this->getTalents($data['id']);
}
[...]

private function getIngameInfo(){
    if ($this->db->query('SELECT "id", "name", "tag", "description" FROM "attribute";', array())){
        while($row = $this->db->fetchAssoc()) $_SESSION['game']['attribute'][] = $row;
    }

    if($this->db->query('SELECT "id, "name", "tag", "description" FROM "skill";', array())){
        while($row = $this->db->fetchAssoc()) $_SESSION['game']['skill'][] = $row;
    }

    if($this->db->query('SELECT "id", "name", "description" FROM "talent";', array())){
        while($row = $this->db->fetchAssoc()) $_SESSION['game']['talent'][] = $row;
    }
}

private function getAttributes($charid){
    if (is_set($this->attributes_array)) return $this->attributes_array;

    $attributes = array();

    if($this->db->query('SELECT "attributeid", "value" FROM "character_attribute" WHERE "characterid" = $1 ORDER BY "attributeid" ASC;', array($charid))){
        while($row = $this->db->fetchAssoc()) $attributes[] = $row;
    }
    $this->attributes_array = $attributes;
    return $attributes_array;
}

private function getSkills($charid){
    $skills = array();

    if($this->db->query('SELECT "skillid", "value" FROM "character_skill" WHERE "characterid" = $1 ORDER BY "skillid" ASC;', array($charid))){
        while($row = $this->db->fetchAssoc()) $skills[] = $row;
    }
    return $skills;
}

private function getTalents($charid){
    $talents = array();

    if($this->db->query('SELECT "talentid", "value" FROM "character_talent" WHERE "characterid" = $1 ORDER BY "talentid" ASC;', array($charid))){
        while($row = $this->db->fetchAssoc()) $talents[] = $row;
    }
    return $talents;
}

If your if or while statements only include 1 operation, I prefer to have it after the operation without using {}. But, since your if statements are really long, I think it's fine to user {} and break those up then.

I would get away from putting everything into $_SESSION, especially information that doesn't need to belong there like game configuration. It doesn't seem (and I wouldn't think) you read-in from the $_SESSION (are you going to read 'talent' type?). Only session information should be there. Create a "$gameConfiguration" array where you store the same as $_SESSION['game'].

getIngameInfo() seems to just be configuration information. Could this be cached? Take the $_SESSION['game'] array, serialize it and store it in a (.gz) file. This would save 3 db calls per page.

Under getAttributes, if there's a chance that this gets called more than once, I would store a $getAttributes array under each class and check if it's already set. This would save duplicate work (though increase memory usage).

I would prepare the queries under "getAttributes", "getSkills" and "getTalents". While they are only going to run at most 4 times per load (4 characters), you are most of the way there with your custom PDO since your queries are already in that style.

PHP processes "" for variables and more. Switching to '', PHP doesn't process the string for variables, it's just takes it at its value.

Don't store $sql query if you don't need that variable. There are times your query needs conditionals so you need to have a variable, and during debugging it's nice to have it so you can display the variable to see if it's working right, but otherwise there's no need.

Does your custom PDO need an empty array passed? Could you use NULL for greater clarity (and less brackets)?

Using quotes on your query for system identifiers is better for the db.

I don't like having conditionals on non-conditional items. That is: if ($this->db->query('SELECT "id", "name", "tag", "description" FROM "attribute";', array())){ where instead you do:

$result = $this->db->query('SELECT "id", "name", "tag", "description" FROM "attribute", array());'
if ($result -> numRows() > 0){
     while($result = $this->db->fetchAssoc())  $_SESSION['game']['attribute'][] = $row;
}

though this depends on how your custom returns/handles queries/results.

Generally, all relatively minor improvements though. Posting the whole class and you might get more useful help.

share|improve this answer
    
You should really avoid the braceless syntax. Someone never having seen it before could easily add more than one operation after such a statement and not understand why it doesn't work. Even for those familiar with it it is still really easy to do. Additionally it is inconsistent with the style for the rest of the application. Consistency in style ensures that the code can more easily be read and scanned. Finally, the syntax itself is usually less legible, though this could be argued. I'd keep those braces unless forced (legacy code base, etc...). –  mseancole Jan 3 '13 at 14:42
    
Many thanks for the help! I hope some of your questions could be answered in my questionedit. My db-class indeed does not need to get an empty array, I can leave this parameter. –  32bitfloat Jan 5 '13 at 0:28
add comment

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.