Jump to content

Ways to cut down this code?


reportingsjr

Recommended Posts

If you havent heard, im making a game right now, but im having slight issues :).Right now im setting up one of the most basic and need things for my game, a bank. Its all working fine, mostly.. except its a massive code so it takes a while to preform it. Can anyone help me cut down this cut while still having it work? I would like it to run quickly, which probably isnt going to happen. I mean, every few times you withdraw and deposit something the speed just goes straight down the hole! If you look at the code you will be like :) , thats a ton of code! Keep in mind, its only for withdrawing and depositing to :).EDIT: okay, it only goes down the hole when you select to deposit "all", then it takes forever to load!code:

if($_GET['act'] == "withdraw"){	$bank_item = explode(".", $bank_items->$_GET['item']);	$amount = $_GET['amount'];	if($_GET['amount'] == "all"){		$amount = $bank_item[1];		}	if($bank_item[1] >= $amount){		for($i = "1";$i <= "40"; $i++){			$item = $items->$i;			if($amount == "0"){				break;				}			if($item == ""){				$inv_updater = "UPDATE `inventory` SET `{$i}` = '{$bank_item[0]}' WHERE `name` = '{$session->username}'";				$result = $database->query($inv_updater) or die(mysql_error());				$amount--;				$bank_item[1]--;			}			}		if($bank_item[1] <= "0"){		$bank_updater = "UPDATE `bank` SET `{$_GET['item']}` = '' WHERE `username` = '{$session->username}'";		$result = $database->query($bank_updater) or die(mysql_error());			}else{		$bank_updater = "UPDATE `bank` SET `{$_GET['item']}` = '{$bank_item[0]}.{$bank_item[1]}' WHERE `username` = '{$session->username}'";		$result = $database->query($bank_updater) or die(mysql_error());		}	}else{		echo "<script type='text/javascript'>alert('You dont have {$_GET['amount']} items to deposit!');</script>";	}}elseif($_GET['act'] == "deposit"){	$amount = $_GET['amount'];	$inv_amount = "0";	for($i = "1";$i <= "40"; $i++){		$item = $items->$i;		if($item == $_GET['item']){			$inv_amount++;		}	}		if($amount == "all"){		$amount = $inv_amount;	}	if($amount <= $inv_amount){		for($i = "1"; $i <= "40"; $i++){			$bank_item = explode(".", $bank_items->$i);			if($bank_item[0] == $_GET['item']){				$total_amount = $amount + $bank_item[1];				$database->query("UPDATE `bank` SET `{$i}` = '{$_GET['item']}.{$total_amount}' WHERE `username` = '{$session->username}'") or die(mysql_error());				//delete amount of items from inventory				$amount_deleted = "0";				for($i = "1"; $i <= "40"; $i++){					if($items->$i == $_GET['item'] && $amount_deleted <= $amount){						$amount_deleted++;						$database->query("UPDATE `inventory` SET `{$i}` = '' WHERE `name` = '{$session->username}'") or die(mysql_error());					}				}				break;			}else{				if($i == "40"){					for($i = "1"; $i <= "40"; $i++){						$bank_item = explode(".", $bank_items->$i);						if($bank_item[0] == ""){							$database->query("UPDATE `bank` SET `{$i}` = '{$_GET['item']}.{$amount}' WHERE `username` = '{$session->username}'");							break;						}						if($i == "40"){							echo "<script type='text/javascript'>alert('You dont have enough bank spaces! Please drop some items!');</script>";						}					}				}			}		}	}else{		echo "<script type='text/javascript'>alert('You dont have {$_GET['amount']} {$_GET['item']}\'s to deposit!');</script>";		}}

Link to comment
Share on other sites

Guest BackWebSerives

The best route you want to go down would be to create a class for this, would make reading and editing of much easier.

Link to comment
Share on other sites

Lol, that's not a lot of code. It's not uncommon for large applications to have single files that are over 100KB of code. Anyway..Your 'deposit all' structure is basically laid out like this:

	if($amount <= $inv_amount){		for($i = "1"; $i <= "40"; $i++){			if(){				$database->query("UPDATE `bank` SET `{$i}` = '{$_GET['item']}.{$total_amount}' WHERE `username` = '{$session->username}'") or die(mysql_error());				for($i = "1"; $i <= "40"; $i++){					if(){						$database->query("UPDATE `inventory` SET `{$i}` = '' WHERE `name` = '{$session->username}'") or die(mysql_error());					}				}				break;			}else{				if($i == "40"){					for($i = "1"; $i <= "40"; $i++){						if(){							$database->query("UPDATE `bank` SET `{$i}` = '{$_GET['item']}.{$amount}' WHERE `username` = '{$session->username}'");							break;						}						if($i == "40"){							echo "<script type='text/javascript'>alert('You dont have enough bank spaces! Please drop some items!');</script>";						}					}				}			}		}	}

First of all, I have to point out the difference between a string and a number. "0" is a string, 0 is a number. If you are using them as numbers (like $i++, which is a numeric operation) then define them as numbers. You might be getting hit with some overhead to convert all the strings to numbers.Anyway, your algorithm is broken down into a FOR loop, which contains an if/else block, inside of each is another FOR loop. Since each loop executes a maximum of 40 times, you are sending a potential maximum of 1600 database queries. Since there is an additional query inside the outer FOR loop, it's a potential maximum of 1640 queries. The queries are what are eating up the time. You might want to do a counter and see exactly how many queries are being executed, but ideally you only want one. It's generally a bad idea to put a query statement in a loop, especially in a nested loop. You're asking for trouble there. You might want to just group all of your queries into one big query string, and then send the entire batch to the server all at once instead of making a separate connection for each one. Or, just restructure your logic or database to make it so that you don't need to send that many.Other then that, it's hard to give constructive comments because you don't have a single comment line in there. It's obvious that a statement like if($bank_item[1] >= $amount) is testing $bank_item[1] and $amount, but there's no explanation as to what $bank_item[1] is. So, without semantic comments on what the different things actually mean, like what it means when $item == $_GET['item'], then it's hard to offer any useful criticism other then the general stuff like don't put a database query in a nested loop.

Link to comment
Share on other sites

Create an account or sign in to comment

You need to be a member in order to leave a comment

Create an account

Sign up for a new account in our community. It's easy!

Register a new account

Sign in

Already have an account? Sign in here.

Sign In Now
×
×
  • Create New...