Code Review Needed

[The Wizard]

Hello:

I'm planing on submitting this as a mod, and will be coding it for the package manager. Before I do though you are invited to review my code, and point out any problems that will cause it not to be accepted as a mod. You are also welcome to point out all the ways I could have coded things better. Please if you point out how I can make it better don't just say you can do this and that - write out the code for everybody to see.

Background info:

This mod is to be used with SMF Shop. If you don't have SMF Shop installed then it wont work. What will this mod do? - it installs a mini gallery in your posts under your avatar. The gallery shows small images of all the items you have bought through the SMF shop mod. As the SMF Shop mod stands now you can buy items for example - a pet rock, but its all text based. Wouldn't it be much cooler if all your users could show off that they own a pet rock?

I have completed the min gallery code, and the code to add cool items to the SMF Shop.

The code below is the last part of the overall mod - removing items.

What good is it to have a min gallery full of images if you can't remove the images?

so Assume there is a column in the database called wizard with the names of the images in it. Also assume I have created a hook called wizard that can be called by ?action=wizard.

Hopefully that should be all the information needed.

Subs-Wizards_Item_Gallery.php

in Source dir

Code Select Expand

<?php

// place this file in the Sources file

    if (!defined('SMF'))
            die('Hacking attempt...');

// Function: wizard hook
 
function wizard_add_hook(&$actionArray)
{

    $actionArray['wizard'] = array('Subs-Wizards_Item_Gallery.php', 'wizard');

} // End of Function wizard hook bracket

// Function Wizard: Loads the wizard template file
 
function wizard ()
{
    
// Globals

global $context;

// this command loads the template file (wizard_item_gallery.template.php)

    loadTemplate('wizard_item_gallery'); 

} // End of Function wizard bracket

// Function showGalleryImages: Show all Images user has in Gallery

function showGalleryImages()
{

    // Globals

global $smcFunc, $txt, $user_info, $context, $boardurl;

// Get the user's information from the database

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

 // Populate the array

 while($row=$smcFunc['db_fetch_assoc']($request1)){

 if(empty($row))
 return false;

 $wizard2 = explode(',', $row['wizard']);

 } // End of While bracket

 echo '<center><h3>', $context['wiz0'], '</h3></center>';

 // Show Users Items in a Gallery

 $i = 0;

 echo '<center><table><tr>';

 foreach($wizard2 as $image)

 {

 // Ignore Items the user does not have

 if (empty($image))
 continue;

        $i++;

// Display Items the user has

echo '<td><a href="?action=wizard&run=one&a='.$image.'"><img src="', $boardurl, '/Sources/shop/wizards_item_image_gallery/small/',$image,'.png" border=0></a></td>';

//To change the number of colums displayed across just change $wiz to the number you need to display

$wiz = 6;

if($i % $wiz ==0)

echo '</tr><tr>';

} // End of foreach bracket

echo '</tr></table></center>';

} // End of Function showGalleryImages bracket

// Function onClick: on Click - link to function

function onClickOne()
{

        if(isset($_GET['run']))$linkchoice=$_GET['run'];

    else $linkchoice = '';

    switch($linkchoice) {

    case 'one':
        showImage();
        break;

case 'two':
        removeImage();
        break;
      
    } // End of switch

} // End of function onClickOne bracket

// Function showImage: show the image the user choose

function showImage()
{

    // Globals

global $smcFunc, $txt, $boardurl, $context;

// Check for inputted image

$image2 = !empty($_GET['a']) ? $smcFunc['htmltrim']($smcFunc['htmlspecialchars']($_GET['a'], ENT_QUOTES)) : '';

    // wiz1 Text says: You have chosen the following item to remove

        echo '<center><h4>', $context['wiz1'], '</h4></center>';

    // Show image that was clicked on

    echo '<center><img src="', $boardurl, '/Sources/shop/wizards_item_image_gallery/small/', $image2,'.png"></center>';

        // wiz2 to wiz4 Text Warning for Idiot Users says: Are you sure? Once you click below the item will be removed forever. If you want the item again you will have to buy it all over.

        echo '<center><h4>', $context['wiz2'], '<br>', $context['wiz3'], '<br>', $context['wiz4'], '</h4></center.';

    // Link to remove item

    echo '<center><br><a href="?action=wizard&run=two&b='.$image2.'"><img src="'.$boardurl.'/Themes/default/images/buttons/removebutton.png"></a></center>';


} // End of function showImage bracket

// Function removeImage: Removes the image name from the wizard column

function removeImage()
{

    // Globals

global $smcFunc, $txt, $user_info, $boardurl;

    // Check inputted image

    $inputted_key = !empty($_GET['b']) ? $smcFunc['htmltrim']($smcFunc['htmlspecialchars']($_GET['b'], ENT_QUOTES)): '';

if(empty($inputted_key))
        return false;
   
    // Get the User's information from the Database

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

// Populate array

list($wizard) = $smcFunc['db_num_rows']($request) > 0 ? $smcFunc['db_fetch_row']($request) : array();

    $smcFunc['db_free_result']($request);

    if (empty($wizard))
        return false;
   
    // Explode & Locate

    $wizard_arr = explode(',', $wizard);

$final_key = array_search($inputted_key, $wizard_arr);

    if (empty($final_key))
        return false;
   
    // Remove Image

    unset($wizard_arr[$final_key]);

    // Update the Members Column

    updateMemberData($user_info['id'], array('wizard' => implode(',', $wizard_arr)));

    // Refresh the Page and Clear the Post Data

    redirectexit($boardurl.'/index.php?action=wizard');

} // End of Function removeImage bracket
?>

wizard_item_gallery.template.php

in themes/default

Code Select Expand

<?php

function template_main () 
{
    // Globals
    
global $smcFunc, $txt, $user_info, $context, $db_prefix, $boardurl;
  
    echo '<html>
          <head>';
  
//  All links on the page will be non-underlined
        echo '
            <style type="text/css">
            a { text-decoration:none }
            </style>';

    // This is a copy of the shop mod links + my added link as calling up from the shop mod is a pain in the ass as the shop mod code is a mess!
    echo'   
            </head> 
            <body>
    <center>
            <table border=5>
        <tr>
    <td>
    <b>SMF Shop Home</b><br><br>
    <a href="' . $boardurl . '/index.php?action=shop">Shop Home</a><br>
            <a href="' . $boardurl . '/index.php?action=shop;do=buy">Buy Stuff</a><br>
            <a href="' . $boardurl . '/index.php?action=shop;do=inv">Your Inventory</a><br>
            <a href="' . $boardurl . '/index.php?action=shop;do=sendmoney">Send Money to Someone</a><br>
            <a href="' . $boardurl . '/index.php?action=shop;do=senditems">Send an Item to Someone</a><br>
            <a href="' . $boardurl . '/index.php?action=shop;do=invother">View Other Members Inventory</a><br>
            <a href="' . $boardurl . '/index.php?action=shop;do=bank">Bank</a><br>
            <a href="' . $boardurl . '/index.php?action=shop;do=trade">Trade Center</a><br>
            <a href="' . $boardurl . '/index.php?action=wizard">Remove Item from Gallery</a><br> 
    </td>
    <td>';  
  
// wiz5 to wiz Text says: SMFSHOP Item Gallery Remover By: The Wizard Version 2.0

    echo '<center><h3>', $txt['wiz5'], '<br>', $txt['wiz6'], '<br>', $txt['wiz7'], '</h3></center>';

    // Show all Images user has in Gallery

    showGalleryImages();

// wiz0 Text says: Click on a item in your gallery to remove

    echo '<center><h3>', $txt['wiz0'], '</h3></center>';

// on Click - link to function

    onClickOne(); 


    // Shows a picture of a wizard in the third column.

    echo '</td>
              <td><img src="' . $boardurl . '/Themes/default/images/wizard2.gif"></td>
          </tr>
              </table>
      </center>
              </body>
              </html>';
  
} // End of function template_main bracket


?>






 

[vbgamer45]

For modification approval you have to make your modification xhtml valid.

Change your <br> tags to <br />
For <img> tags must have alt="" and a closing /

[emanuele]

The part "What does that mean?" of this post is still valid and is important for approval.

You should never have an "echo" in a source file (well, there *are* exception, but is not the case of this mod ).
You should never call a function in a template file from source
You should never call a function in a source file from the template

So, at the moment, the only properly coded functions are:
* wizard_add_hook
* wizard
* removeImage

That said, let's see the most problematic.
function showGalleryImages: here you have a query and you are "echo"ing template. And you are calling showGalleryImages from the template file. Both these are bad.
What to do?
You have to move the query to the function wizard, this entire block in particular:

Code Select Expand

// Get the user's information from the database

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

// Populate the array

while($row=$smcFunc['db_fetch_assoc']($request1)){

if(empty($row))
return false;

$wizard2 = explode(',', $row['wizard']);

} // End of While bracket
Once this is there, you can add *after* that block a line similar to:

Code Select Expand

$context['wizard2'] = $wizard2;
then you can move all the rest of the function (all the "echo") to the function template_main in the position where now you call the showGalleryImages function.
While doing this, you'll change this line:

Code Select Expand

foreach($wizard2 as $image)
to

Code Select Expand

foreach($context['wizard2'] as $image)
do you see how the two functions are now linked? $context is present both in "wizard" and in "template_main" due to the "global $context" piece and the values in there are accessible from inside both the functions.

That is one. Now let's see function showImage: this has basically only "echo"s, so it belongs to the template.

Now the tricky one: function onClickOne. Here you really have to decide what you want to do: half of the function creates a template, the other half does a "source" part (remove the image). Okay, for now let's leave it as it is, but start thinking about it (hint: you have to split it similarly to showGalleryImages ).

Code Select Expand

    echo '<html>
          <head>';
 
//  All links on the page will be non-underlined
        echo '
            <style type="text/css">
            a { text-decoration:none }
            </style>';

    // This is a copy of the shop mod links + my added link as calling up from the shop mod is a pain in the ass as the shop mod code is a mess!
    echo'  
            </head>
            <body>
You don't need this.
The tags <html>, <head>, <body> should be created by SMf itself.
To inject the <style> into the header, there is a "trick": in the function wizard write:

Code Select Expand

$context['html_headers'] .= '
            <style type="text/css">
            a { text-decoration:none }
            </style>';



Finally: all the hard-coded text like:

Code Select Expand

<b>SMF Shop Home</b><br><br>
this is a no-go for the approval.
You have to replace it with $txt variables and if needed a language file, so for example:

Code Select Expand

<b>', $txt['smf_shop_home'], </b><br><br>
and then add in a language file the line:

Code Select Expand

$txt['smf_shop_home'] = 'SMF Shop Home';



After that there are still few details, but better start with the major things.

Learning a programming language from scratch is not easy, keep up and continue, you are improving!

[The Wizard]

Hello all:

Below are all the changes that were suggested, and I did change a few things myself in the code hopefully for the better. I dropped the function onClickone. Moved a few things around, and used if statements. So please feel free to point out any new errors or reasons why the code wont be accepted as a mod.

Thanks

Wiz

Subs-Wizards_Item_Gallery.php

in Source dir

Code Select Expand

<?php

// place this file in the Sources file

    if (!defined('SMF'))
            die('Hacking attempt...');

// Function: wizard hook
 
function wizard_add_hook(&$actionArray)
{

    $actionArray['wizard'] = array('Subs-Wizards_Item_Gallery.php', 'wizard');

} // End of Function wizard hook bracket

// Function Wizard: Loads the wizard template file
 
function wizard ()
{
    
// Globals

global $smcFunc, $txt, $user_info, $context, $boardurl;

// this command loads the template file (wizard_item_gallery.template.php)

    loadTemplate('wizard_item_gallery');

// add html headers and make all links on the page non-underlined

    $context['html_headers'] .= '
            <style type="text/css">
            a { text-decoration:none }
            </style>';

    // Show all Images user has in Gallery

    // Get the user's information from the database

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

 // Populate the array

 while($row=$smcFunc['db_fetch_assoc']($request1)){

 if(empty($row))
 return false;

 $wizard2 = explode(',', $row['wizard']);

 } // End of While bracket
 
$context['wizard2'] = $wizard2;

// Removes the image name from the wizard column

    // Check inputted image

    $inputted_key = !empty($_GET['b']) ? $smcFunc['htmltrim']($smcFunc['htmlspecialchars']($_GET['b'], ENT_QUOTES)): '';

if($inputted_key){
   
    // Get the User's information from the Database

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

// Populate array

list($wizard) = $smcFunc['db_num_rows']($request) > 0 ? $smcFunc['db_fetch_row']($request) : array();

    $smcFunc['db_free_result']($request);

    if (empty($wizard))
        return false;
   
    // Explode & Locate

    $wizard_arr = explode(',', $wizard);

$final_key = array_search($inputted_key, $wizard_arr);

    if (empty($final_key))
        return false;
   
    // Remove Image

    unset($wizard_arr[$final_key]);

    // Update the Members Column

    updateMemberData($user_info['id'], array('wizard' => implode(',', $wizard_arr)));

    // Refresh the Page and Clear the Post Data

    redirectexit($boardurl.'/index.php?action=wizard');

    } // End of if($inputted_key) bracket

} // End of Function wizard bracket

?>

wizard_item_gallery.template.php

in themes/default

Code Select Expand

<?php

function template_main () 
{
    // Globals
    
global $smcFunc, $txt, $user_info, $context, $db_prefix, $boardurl;
  
    // This is a copy of the shop mod links + my added link as calling up from the shop mod is a pain in the ass as the shop mod code is a mess!
    echo'   
            <center>
            <table border=5>
        <tr>
    <td>
    <b>', $txt['wizard_smf_shop_home'], '</b><br /><br />
    <a href="' . $boardurl . '/index.php?action=shop">', $txt['wizard_shop_home'], '</a><br />
            <a href="' . $boardurl . '/index.php?action=shop;do=buy">', $txt['wizard_buy_stuff'], '</a><br />
            <a href="' . $boardurl . '/index.php?action=shop;do=inv">', $txt['wizard_your_inventory'], '</a><br />
            <a href="' . $boardurl . '/index.php?action=shop;do=sendmoney">', $txt['wizard_send_money_to_someone'], '</a><br />
            <a href="' . $boardurl . '/index.php?action=shop;do=senditems">', $txt['wizard_send_an_item_to_someone'], '</a><br />
            <a href="' . $boardurl . '/index.php?action=shop;do=invother">', $txt['wizard_view_other_members_inventory'], '</a><br />
            <a href="' . $boardurl . '/index.php?action=shop;do=bank">', $txt['wizard_bank'], '</a><br />
            <a href="' . $boardurl . '/index.php?action=shop;do=trade">', $txt['wizard_trade_center'], '</a><br />
            <a href="' . $boardurl . '/index.php?action=wizard">', $txt['wizard_remove_item_from_gallery'], '</a><br /> 
    </td>
    <td>';  
  
// Text says: SMFSHOP Item Gallery Remover By: The Wizard Version 2.0

    echo '<center><h3>', $txt['wizard_smfshop_item_gallery_remover'], '<br />', $txt['wizard_by_the_wizard'], '<br />', $txt['wizard_version'], '</h3></center>';

    // Show all Images user has in Gallery

echo '<center><h3>', $txt['wizard_click_on_item_in_your_gallery_to_remove'], '</h3></center>';

 // Show Users Items in a Gallery

 $i = 0;

 echo '<center><table><tr>';

 foreach($context['wizard2'] as $image)

 {

 // Ignore Items the user does not have

 if (empty($image))
 continue;

        $i++;

// Display Items the user has

echo '<td><a href="?action=wizard&a='.$image.'"><img src="', $boardurl, '/Sources/shop/wizards_item_image_gallery/small/',$image,'.png" border=0 alt="', $image, '" /></a></td>';

//To change the number of colums displayed across just change $wiz to the number you need to display

$wiz = 6;

if($i % $wiz ==0)

echo '</tr><tr>';

} // End of foreach bracket

echo '</tr></table></center>';

// Show the Image the user choose

// Check for inputted image

$image2 = !empty($_GET['a']) ? $smcFunc['htmltrim']($smcFunc['htmlspecialchars']($_GET['a'], ENT_QUOTES)) : '';

if($image2){

    // Text says: You have chosen the following item to remove

        echo '<br /><center><h4>', $txt['wizard_you_have_chosen_the_following_item_to_remove'], '</h4></center><br />';

        // Show image that was clicked on

        echo '<center><img src="', $boardurl, '/Sources/shop/wizards_item_image_gallery/small/', $image2,'.png" alt="', $image2, '" /></center>';

        // Text Warning for Idiot Users says: Are you sure? Once you click below the item will be removed forever. If you want the item again you will have to buy it all over.

        echo '<br /><center><h4>', $txt['wizard_are_you_sure'], '<br />', $txt['wizard_warning1'], '<br />', $txt['wizard_warning2'], '</h4></center>';

        // Link to remove item

       echo '<center><br /><a href="?action=wizard&b='.$image2.'"><img src="'.$boardurl.'/Themes/default/images/buttons/removebutton.png" alt="remove button" /></a></center><br />';

} // End of if($image2) bracket

// Shows a picture of a wizard in the third column.

    echo '</td>
              <td><img src="' . $boardurl . '/Themes/default/images/wizard2.gif" alt="wizard image" /></td>
          </tr>
              </table>
      </center>';
  
} // End of function template_main bracket

?>

Modifications.english.php

Code Select Expand

// Begin Wizard Image Gallery code
$txt['wizard_click_on_item_in_your_gallery_to_remove'] = 'Click on a item in your gallery to remove';
$txt['wizard_you_have_chosen_the_following_item_to_remove'] = 'You have chosen the following item to remove';
$txt['wizard_are_you_sure'] = 'Are you sure?';
$txt['wizard_warning1'] = 'Once you click below the item will be removed forever.';
$txt['wizard_warning2'] = 'If you want the item again you will have to buy it all over.';
$txt['wizard_smfshop_item_gallery_remover'] = 'SMFSHOP Item Gallery Remover';
$txt['wizard_by_the_wizard'] = 'By: The Wizard';
$txt['wizard_version'] = 'Version 2.0';
$txt['wizard_smf_shop_home'] = 'SMF Shop Home';
$txt['wizard_shop_home'] = 'Shop Home';
$txt['wizard_buy_stuff'] = 'Buy Stuff';
$txt['wizard_your_inventory'] = 'Your Inventory';
$txt['wizard_send_money_to_someone'] = 'Send Money to Someone';
$txt['wizard_send_an_item_to_someone'] = 'Send an Item to Someone';
$txt['wizard_view_other_members_inventory'] = 'View Other Members Inventory';
$txt['wizard_bank'] = 'Bank';
$txt['wizard_trade_center'] = 'Trade Center';
$txt['wizard_remove_item_from_gallery'] = 'Remove Item from Gallery';
// End Wizard Image Gallery code




 

[The Wizard]

 Well I just tested this code and it does not not work, and I can't figure out why. The image you choose shows up, but when you try to remove it does not remove. Can anybody shed some light on this or do I need the onClick stuff after all?

Wiz

[The Wizard]

 

[emanuele]

What is not working exactly?
ATM I'm in a hurry, cannot read the code deeply...