database code help needed

[The Wizard]

Hello:

I could use some help with the code for the new shop mod I'm working on. Below is what I have so far.
I'm hoping their is a way to combine the "Get the user's information from the database" code, and the "Get the item's information" code?

Feel free to point out all the mistakes I have made, but be aware this is a first rough draft.

Wiz

Koodi [Valitse] Laajenna


function TestUseItem()
{
global $smcFunc, $context, $txt;

// Inv2 - Using an item, and user needs to enter some input
if (isset($_GET['do']) && $_GET['do'] == 'inv2')
{
// Get the user's information from the database
$result1 = $smcFunc['db_query']('', "
SELECT wizard
FROM {db_prefix}members
WHERE id_member = {int:current_member}
LIMIT 1", array(
'current_member' => $user_info['id']
)
);

// fetch their information
$row1 = $smcFunc['db_fetch_assoc']($result1);

// Get the item's information
$result2 = $smcFunc['db_query']('', "
SELECT it.module, it.can_use_item, it.info1, it.info2, it.info3,
it.info4, inv.ownerid, it.image,
FROM {db_prefix}shop_inventory AS inv, {db_prefix}shop_items AS it
WHERE inv.id = {int:id} AND inv.itemid = it.id
LIMIT 1", array(
'id' => $_GET['id']
));

$row2 = $smcFunc['db_fetch_assoc']($result2);
$smcFunc['db_free_result']($result2);

// Trying to use other person's item? Nope, not here ;)
if ($row2['ownerid'] !== $user_info['id'])
fatal_lang_error('shop_use_others_item');

$context['wizard_image'] = $row2['image'];

// Get Image from SMF Shop Users Inventory
$wizard_image = $context['wizard_image'];

// Strips out .png extension
$wizard_image = substr($wizard_image, 0, strrpos($wizard_image, '.'));

// add your value to the array - in this case the name of the image
$B = $row['wizard'] . (!empty($row['wizard']) ? ',' : '') . $wizard_image;

// Save all the orginal image names plus the new value
updateMemberData($user_info['id'], array(
'wizard' => $B
));

// Free the result
$smcFunc['db_free_result']($result);

// Return
return '<br><h2>Your Item should now show up in your posts - Wiz</h2><br />';
}
}


[Arantor]

Well... since there is absolutely nothing in common between the two queries, how could you possibly combine them? A query is asking a question of a set of data.

But while we're at it... let's see. You're using $user_info without actually bringing it into scope (add it to the global declaration) so that part isn't going to work very well.

You're using a $_GET variable without validating that it is in fact an integer. Best bet is to cast to integer and check it's greater than zero (which means you get to eliminate nonsense values as well as rogue negative values)

You're hoping to cut a query down... well, you can only do that if there is something that connects the data, as mentioned, but in this case I can save you some effort.

Everything, and I do mean everything, about the current user that's in smf_members is preloaded at startup. So if wizard is a column in the members table, it will have been loaded already into $user_settings['wizard'] - note that this is not the same as $user_info. You will need to pull it into scope (add it to the global declaration) but then you can use it.

[The Wizard]

Hopefully I have understood what you were trying to explane and I have made some changes the most noticeable is that I have decided to pass the image name instead of the id. Feel free to point out all the mistakes I have made.

Wiz

Koodi [Valitse] Laajenna


function TestUseItem()
{
global  $user_info, $user_settings, $smcFunc;

// Inv2 - Using an item, and user needs to enter some input
if (isset($_GET['do']) && $_GET['do'] == 'inv2')
{
if(empty($_GET['image']))
{
fatal_lang_error('image_file_check_error', false);
}

// Get Image names from the users wizard column
$wizard_image = $_GET['image'];

// add your value to the array
$B = $user_settings['wizard'] . (!empty($row['wizard']) ? ',' : '') . $wizard_image;

// Save all the orginal image names plus the new value
updateMemberData($user_info['id'], array(
'wizard' => $B
));

// Free the result
$smcFunc['db_free_result']($result);

// Everything went well and so we let the user know
$context['shop_buy_message'] = '<br><h2>Your Item should now show up in your posts - Wiz</h2><br />';

// Use the "use item" template
$context['sub_template'] = 'useitem';
}
}


[Matthew K.]

One thing I did notice in the code you posted in the first topic...you never free the results of $result1, which...isn't the end of the world, but might be nice to do. And you also free the results of "$result", which is never defined...

[The Wizard]

Hello:

This is my latest version and it does work. Feel free to point out all the mistakes I have made and what I can improve.

Wiz

Koodi [Valitse] Laajenna


// Inv2 - Using an item, and user needs to enter some input
if (isset($_GET['do']) && $_GET['do'] == 'inv2')
{
global  $user_info, $user_settings, $smcFunc;

// Make sure input is numeric
$_GET['id'] = (int) $_GET['id'];

if(empty($_GET['image']))
{
fatal_lang_error('image_file_check_error', false);
}

// Get Image names from the users wizard column
$wizard_image = $_GET['image'];

// Strips out .png extension
$wizard_image = substr($wizard_image, 0, strrpos($wizard_image, '.'));

// Get the user's information from the database and adds a comma limiter
$result = $smcFunc['db_query']('', "
SELECT wizard
FROM {db_prefix}members
WHERE id_member = {int:current_member}
LIMIT 1", array(
'current_member' => $user_info['id']
)
);

// fetch their information
$row = $smcFunc['db_fetch_assoc']($result);

// add your value to the array
$B = $row['wizard'] . (!empty($row['wizard']) ? ',' : '') . $wizard_image;

// Save all the orginal image names plus the new value
updateMemberData($user_info['id'], array(
'wizard' => $B
));

// Free the result
$smcFunc['db_free_result']($result);

// Everything went well and so we let the user know
$context['shop_buy_message'] = '<br><h2>Your Item should now show up in your posts - Wiz</h2><br />';

// Now lets remove the item from the user's inventory
$result = $smcFunc['db_query']('', "
SELECT it.module, it.can_use_item, it.delete_after_use,
it.info1, it.info2, it.info3, it.info4, inv.ownerid
FROM ({db_prefix}shop_inventory AS inv, {db_prefix}shop_items AS it)
WHERE inv.id = {int:id} AND inv.itemid = it.id
LIMIT 1", array(
'id' => $_GET['id']
));

$row2 = $smcFunc['db_fetch_assoc']($result);
$smcFunc['db_free_result']($result);


if ($row2['delete_after_use'] == 1)
$smcFunc['db_query']('', "
DELETE FROM {db_prefix}shop_inventory
WHERE id = {int:id}
LIMIT 1", array(
'id' => $_GET['id']
));

// Use the "use item" template
$context['sub_template'] = 'useitem';
}


[Arantor]

1. $_GET['id'] will, if undefined, throw an error. Suggestion:

Koodi [Valitse] Laajenna

$_GET['id'] = isset($_GET['id'] ? (int) $_GET['id'] : 0;

What would happen if a value of 0 were ever fed into the query? Well, you use the query to get $row2 but if it were 0, it seems to me as though it would fail that query - and thus $row2 would throw errors because the fetch_assoc would return false and every single test thereafter would fail too.

It's not just enough to validate that it is the type you expect to find, but also make sure that the value is within bounds that are reasonable for the use in question. If 0 is not a valid number for that variable, throw it out at the earliest opportunity.

I also see you did not take my advice about removing an entire query.

[The Wizard]

I did not even think about zero being a problem so this is good to know.

As for removing the one query I did try, but for some reason a delimiter was not added to the table.
Instead of getting - item1, item2, item3, etc... I was getting item1item2item3....

I do have a weird question, and I expect this will show my ignorance, but if I don't ask then I wont learn.
In the query below we have "it.module, it.can_use_item, it.delete_after_use, it.info1, it.info2, it.info3, it.info4, inv.ownerid"
Do I have to use all these or can I remove "it.info1, it.info2, it.info3, it.info4,"?

Thanks

Wiz

Koodi [Valitse] Laajenna


$result = $smcFunc['db_query']('', "
SELECT it.module, it.can_use_item, it.delete_after_use,
it.info1, it.info2, it.info3, it.info4, inv.ownerid
FROM ({db_prefix}shop_inventory AS inv, {db_prefix}shop_items AS it)
WHERE inv.id = {int:id} AND inv.itemid = it.id
LIMIT 1", array(
'id' => $_GET['id']
));


[Arantor]

As for removing the one query I did try, but for some reason a delimiter was not added to the table.
Instead of getting - item1, item2, item3, etc... I was getting item1item2item3....

Not being funny but I shouldn't actually have to be explaining this.

My suggestion, take this:

Koodi [Valitse] Laajenna

$result = $smcFunc['db_query']('', "
SELECT wizard
FROM {db_prefix}members
WHERE id_member = {int:current_member}
LIMIT 1", array(
'current_member' => $user_info['id']
)
);

// fetch their information
$row = $smcFunc['db_fetch_assoc']($result);

$B = $row['wizard'] . (!empty($row['wizard']) ? ',' : '') . $wizard_image;

Replace with:

Koodi [Valitse] Laajenna


$B = $user_settings['wizard'] . (!empty($user_settings['wizard']) ? ',' : '') . $wizard_image;

I'm sorry but I'm not here to entirely hold your hand. The suggestion was to replace $row['wizard'] with $user_settings['wizard']. It sort of follows that you have to do that with all the places you originally used $row['wizard'] for. Including the delimiter.

[The Wizard]

The suggestion was to replace $row['wizard'] with $user_settings['wizard']. It sort of follows that you have to do that with all the places you originally used $row['wizard'] for. Including the delimiter.

What can I say?
Your right I missed the second part of that line of code and so my attempt failed to work. So I fell back and used the query instead.

Should I have tryed harder?
In retrospect yes I should have.

Am I big enough to say I don't know enough? Yes.

Will I ever stop trying to learn enough? - NO.

Will I ever get better at it? I don't know, but I hope so.

Thanks again for all your help. I really do appreciate it. Your amazing at php and your welcome to scold me every time I make a mistake - I can take it.

Wiz

Below is the completed code that does work.

Koodi [Valitse] Laajenna


// Inv2 - Using an item, and user needs to enter some input
if (isset($_GET['do']) && $_GET['do'] == 'inv2')
{
global  $user_info, $user_settings, $smcFunc;

// Make sure id input is numeric and not zero
$_GET['id'] = isset($_GET['id']) ? (int) $_GET['id'] : 0;

if(empty($_GET['image']))
{
fatal_lang_error('wizards_shop_file_check_error', false);
}

// Get Image names from the users wizard column
$wizard_image = $_GET['image'];

// Strips out .png extension
$wizard_image = substr($wizard_image, 0, strrpos($wizard_image, '.'));

// Get the user's information from the database and adds a comma limiter
$B = $user_settings['wizard'] . (!empty($user_settings['wizard']) ? ',' : '') . $wizard_image;

// Save all the orginal image names plus the new value
updateMemberData($user_info['id'], array(
'wizard' => $B
));

// Free the result
$smcFunc['db_free_result']($result);

// Everything went well and so we let the user know
$context['shop_buy_message'] = '<br><h2>Your Item should now show up in your posts - Wiz</h2><br />';

// Now lets remove the item from the user's inventory
$result = $smcFunc['db_query']('', "
SELECT it.module, it.can_use_item, it.delete_after_use,
it.info1, it.info2, it.info3, it.info4, inv.ownerid
FROM ({db_prefix}shop_inventory AS inv, {db_prefix}shop_items AS it)
WHERE inv.id = {int:id} AND inv.itemid = it.id
LIMIT 1", array(
'id' => $_GET['id']
));

$row2 = $smcFunc['db_fetch_assoc']($result);
$smcFunc['db_free_result']($result);


if ($row2['delete_after_use'] == 1)
$smcFunc['db_query']('', "
DELETE FROM {db_prefix}shop_inventory
WHERE id = {int:id}
LIMIT 1", array(
'id' => $_GET['id']
));

// Use the "use item" template
$context['sub_template'] = 'useitem';
}


[Arantor]

See, that's the bit that annoys me, really. You do have what it takes, but the true secret of programming is about self discipline. If it doesn't work, it doesn't work for a reason and invariably that reason is that what you told it to do is not what you thought you told it to do.

Pro-tip: step back from the computer, go and make a cup of whatever you drink, and empty your mind of all preconceived notions. Then look at the code again but don't tell yourself 'that bit should do x'. Ask yourself - read it out loud if you have to - what the code is *actually* doing. Not what you think you told it, because the computer is a mindless tool for doing things quickly. It will do precisely what you ask of it, nothing more, nothing less.