PHP (with a bit of MySQL) code optimization/rewrite

[JranZu]

Hi all, I've been out of the coding world for awhile and need some help writing a bit cleaner code. I was wondering if anybody could suggest a better way of re-writing the code below. I am sure there is a much better way of writing it. The general idea is to query the database to see if an item is equipped. It needs to search in each of the 9 spots which may or may not return an itemID. If it returns an item it needs to assign that itemID to a specific variable, otherwise it needs to assign 0 to that variable.

 

Any advice would be great, thanks.

 

for ($i = 1; $i <= 9; $i++){
   $result = mysql_query ("SELECT itemID FROM inventory WHERE characterID = $charID AND equipmentSlot = $i LIMIT 1;");
   if (!$row = mysql_fetch_array($result)){
      $row["itemID"] = 0;
   }
   switch($i){
      case(1):
         $this->mainHandID = $row["itemID"];
         break;
      case(2):
         $this->offHandID = $row["itemID"];
         break;
      case(3):
         $this->armorID = $row["itemID"];
         break;
      case(4):
         $this->glovesID = $row["itemID"];
         break;
      case(5):
         $this->helmID = $row["itemID"];
         break;
      case(6):
         $this->bootsID = $row["itemID"];
         break;
      case(7):
         $this->ringOneID = $row["itemID"];
         break;
      case(:
         $this->ringTwoID = $row["itemID"];
         break;
      case(9):
         $this->necklaceID = $row["itemID"];
         break;
      }
}

[Mchl]

Start with removing query from the loop, like this:

 

$query = "SELECT itemID, equipmentSlot FROM inventory WHERE characterID = $charID";
$result = mysql_query($query);
while ($row = mysql_fetch_array($result)) {
switch($row['equipmentSlot']){
      case(1):
         $this->mainHandID = $row["itemID"];
         break;
      case(2):
         $this->offHandID = $row["itemID"];
         break;
      case(3):
         $this->armorID = $row["itemID"];
         break;
      case(4):
         $this->glovesID = $row["itemID"];
         break;
      case(5):
         $this->helmID = $row["itemID"];
         break;
      case(6):
         $this->bootsID = $row["itemID"];
         break;
      case(7):
         $this->ringOneID = $row["itemID"];
         break;
      case(:
         $this->ringTwoID = $row["itemID"];
         break;
      case(9):
         $this->necklaceID = $row["itemID"];
         break;
      }
}

[corbin]

Edit:  Went and did something in the middle of writing this....  Mchl beat me to it, but posting this anyway.

 

 

If you wanted to minimize SQL queries, you could do it this way:

 

$q = mysql_query("SELECT itemID, equipmentSlot from inventory WHERE characterID = $charID AND equipmentSlot > 0");
if(mysql_num_rows($q)) {
   while($r = mysql_fetch_assoc($q)) {
       switch($r['equipmentSlow']) {
               case 1:
                   $this->mainHandID = $r['itemID'];
               break;

               case 2:
                   $this->offHandID = $r['itemID'];
               break;
               // so on.....
       }
   }
}

 

 

You could also store where to store it in an array too, with the keys of the array corrosponding to the equipmentSlot, but that would be overkill.

[Mchl]

Well I think, that changing equipmentSlot type to ENUM('mainHand','offHand',...) could actually be a good idea.

[JranZu]

Thanks... I was hoping to get away from the switch statement and use a foreach or for loop... But maybe the switch is the best way. The reason I have the original for loop in there is so that I can limit the query. No a big deal, but I like using limits; it probably is faster and less recourse intensive just to drop it.

[JranZu]

I've never worked with ENUM -- any advice on how to go forward with that would be great...