Is this code correct?

[Vampy]

Hi! I was wondering if anyone here could help me look through the following lines of code and tell me whether it is corrrectly written for what I want it to do. Basically, what I need the code to do is to compare $itemfound with the user item. If the user already has the item, the code will get a different $itemfound until the $itemfound is something that the user does not have.

Code: Select all

do {
		// what item did the user won??
			if ($itemtype == 'C') 
			{ 
				$items_array = explode("##", $items_ch);
				$newrand = rand(1,10); 
				$itemfound = $items_array[$newrand];
			} 
			if ($itemtype == 'UC') 
			{ 
				$items_array = explode("##", $items_ch);
				$newrand = rand(1,10); 
				$itemfound = $items_array[$newrand];
			} 
			if ($itemtype == 'R') 
			{ 
				$items_array = explode("##", $items_ch);
				$newrand = rand(1,10); 
				$itemfound = $items_array[$newrand];
			} 
			if ($itemtype == 'VR') 
			{ 
				$items_array = explode("##", $items_ch);
				$newrand = rand(1,10); 
				$itemfound = $items_array[$newrand];
			}
			if (preg_match("/^@([A-Za-z]+[\_*|\s*|\-*|\&*|\(*|\)*]*)+/", $itemfound, $matches)) {
				$temp_avatar_name = preg_replace("\@", "", $matches[0]);
				$find_item_sql = "select * from phpbb_shops where name=".$item_avatar_name;
				if ( !($find_item_result = $db->sql_query($find_item_sql)) )
				{
					message_die(GENERAL_MESSAGE, 'Fatal Error: '.mysql_error());
				}
				$find_item = mysql_fetch_array($find_item_result);
				if (mysql_num_rows($find_item_result) < 1) { message_die(GENERAL_MESSAGE, 'No such item exists!'); }
				$temp_find_item = "ß".$find_item['item_image_dir']."Þ";
				$temp_user_bought_avatar = $row['user_bought_avatar'];
				if (substr_replace($temp_user_bought_avatar, $temp_find_item, strpos($temp_user_bought_avatar, $temp_find_item), strlen($temp_find_item))) {
					$item_repeat = 1;
				}
				else {
					$item_repeat = 0;
				}
			}
			else {
				$item_repeat = 0;
			}
		}
		while ($item_repeat != 0);

Did I write the do and while codes properly and whether does it go into infinte loop? Please tell me if there is a more efficient way to wrote the codes to achieve what I need.

Thanks.

[Ptirhiik]

If you preg_match is always verified, you never go outside your loop. preg_match_all() won't be the good one to use here to avoid a non-itered loop ?

[Vampy]

Hi! The preg_match will only be verified when the $itemfound has a particular value. If that is true then it will go and match with the user items. If that is also a match, then $item_repeat will be set to 1 and the loop will execute the previous codes again to obtain a new $itemfound value. If however, preg_match is not successful, then it will set the $item_repest to 0 and will exit the loop. The same goes if although preg_match is valid but if the user has no such item then, it will also exit the loop.

Are you saying that the codes I've written will not work as I want it to (i.e. no matter what happens the loop will go into infinity)? You mentioned that preg_match_all? Ididn't use that in my codes. Are you saying to use it instead of just preg_match?

Thanks for your kind input.

[Ptirhiik]

Yep, I think the problem stands there. Actually, it is more a way to work : you should never use a loop for which you don't know how many iterations will be necessary to go outside. The only exception is a sequential read of a list (meaning no jump in from one item to another, breaking the flow).

A safer approach will be so :
- determine the number of iterations,
- loop (with ie for rather than while)

Doing so your exit condition is know before enter the loop, and it will makes your algo clearer (so less chances to have an error in it).

[Vampy]

Hi! I know what you mean. But the problem lies with the fact that I indeed have no idea how many iterations it would take to get a $itemfound for which the user does not have in his account.

The whole point is that I do not want to give $itemfound to the user for which the user already have. No point in him/her having the same item in his inventory. As I understand with the "for" loop you will need to kow the eact number of iterations but in this case, I will never know the exact iterations needed.

Thank you for your input. I can't see another way to achieve what I need and would appreciate if you could enlighten me. I wonder whether "case:" can help me with achieving what I need. What do you think?

[Ptirhiik]

You know the number of items the user have. If I understood your purpose, you want do add a new if it doesn't stand in the existing ones. Getting first the list of items belonging to this user will gives you the number of items to check with your new item. Doing so you will :
- remove your loop ( only looping on the rand() ),
- reduce greatly the number of needed sql requests.

[Vampy]

I hail to the almighty. Thank you so much. I think I now understand what you are saying.