Can this code be improved??

NCC1701

Hi guys, ive spent ages trying to get this script to work and it finally does, these fuctions are called by two different php files in order to display the forms.

I was just wondering if anything can be taken out that isnt really needed or if there is the possibility of splitting some of the code into separate functions in order to split the HTML from php??

also if any of you could be so kind as to add comments to my code because im unsure how some of it works.

<?php

function display_menu_selection_form()
{
  // create connection 
      $conn = db_connect();
      if (!$conn)
      return false;

  // create SQL statement 
  $query = "SELECT * 
            FROM menu";
  // execute SQL query and get result 
  $sql_result = mysql_query($query,$conn);

  // format results by row
  while ($row = mysql_fetch_array($sql_result)) 
  { 
    $menu  = $row["menu_name"]; 
    $option_block .= "<option value=\"$menu\">$menu</option>"; 
  } 

?>
  <form method="post" action="../test/menu_edit.php">
  <table align="center" border="0">
  <tr>
    <td>
	<select name="menu_name">
	<?php echo "$option_block"; ?>
	</select> 
	</td>
  </tr>
  <tr>
    <td><input type="submit" value="Sumbit" name="Submit"></td>
  </tr>
  </table>
  </form>
<?php 
};

function display_menu_edit_form()
{
// create connection 
      $conn = db_connect();
      if (!$conn)
      return false;

  // create SQL statement 
  $query = "SELECT menu_id, menu_name, menu_text 
            FROM menu 
			WHERE menu_name = '$_POST[menu_name]' ";
  // execute SQL query and get result 
  $sql_result = mysql_query($query,$conn);

  // format results by row
  while ($row = mysql_fetch_array($sql_result)) 
  {
  $menu_name = $row["menu_name"];
  $menu_text = $row["menu_text"];
  } 
?>
  <form method="post" action="">
  <table align="center">
   <tr>
     <td colspan=2>Edit Menu Contents</td>
   <tr>
     <td>Menu Name</td>
     <td><input type="text" name="menu_name" value="<? echo $menu_name ?>"></td></tr>
   <tr>
     <td>Menu Content</td>
     <td>
	 <textarea>
	 <? echo $menu_text ?>
	 </textarea>
	 </td>
   </tr>
   <tr>
     <td colspan=2 align=center>
     <input type="submit" value="Update"></td>
   </tr>   

 </table></form>
<?php
};
?>

as yet the update button does not do anything by the way

any help would be appreciated
NCC1701

axo

first of all, you should NEVER EVER use "echo" or "print" inside functions or methods, except if you temporarily want some output debugging. learn that the output of a function is always done with "return".
much more ugly is your method to mix up "pure" html with php code. (by using <?php $myContent = ""; ?><html><title><?php echo $myContent; ?></title> ... and so on.)

to separate code from html, you could put the html into a separate file, e.g. form.tpl and put placeholders (e.g. in the form {myPlaceHolder} ) at the position where php should put its output in.

on php-side, you load the form.tpl in with fopen, fread, fclose e.g. in a string variable $template. then create an array

$trans = array(
 '{myPlaceHolder}' => "myPlaceHolderContent",
 '{mySecondPlaceHolder}' => 'mySecondPlaceHolderContent',
);

and when you're done with that you replace the placeholders with the php-generated content.

return strtr($template,$trans);

by this, you separate html from php.

xen01

first of all, you should NEVER EVER use "echo" or "print" inside functions or methods, except if you temporarily want some output debugging. learn that the output of a function is always done with "return".

Hello Axo or to anyone..

Given the Data Above how would u implement this? could you please write an example?

LordShryku

Couple of points to note here:
1. Changing the SQL query to only select the field you need. Why create overhead and confusion from selecting all fields when you only need one?
2) The check for rows returned. It's a safe guard saying "only do this if we actually have results to work with".
3) The returns instead of display. I personally write functions both ways, so I won't comment on the "ONLY use return! NEVER echo!". It's a matter of how you want to write the function. I've only used returns here for example. Now, there's a couple way you could return this also. I chose to use false as the default return, which more or less signifies that either something went wrong, or no rows returned. You could also just return and empty array, for the sake of consistancy. Then when you call the function, use a count to determine if it should display or not.

<?php
function display_menu_selection_form() {
   $conn = db_connect();
   if($conn) {
      $results = array();
      $query = "SELECT menu_name
                 FROM menu";
      $sql_result = mysql_query($query, $conn);
      if(mysql_num_rows($sql_result) > 0) {
         while ($row = mysql_fetch_array($sql_result)) { 
            $results[] = $row['menu_name']; 
         } 
         return $results;
      }
   }
   return false;
}

if($menu = display_menu_selection_form()) {
?>
<form method="post" action="../test/menu_edit.php">
  <table align="center" border="0">
    <tr>
      <td><select name="menu_name">
<?php
   foreach($menu as $m) {
?>
            <option value="<?php echo $m; ?>"><?php echo $m; ?></option>
<?php
   }
?>
          </select>
      </td>
    </tr>
    <tr>
      <td><input type="submit" value="Sumbit" name="Submit"></td>
    </tr>
  </table>
</form>
<?php 
}
?>

onion2k

Originally posted by axo
first of all, you should NEVER EVER use "echo" or "print" inside functions or methods, except if you temporarily want some output debugging. learn that the output of a function is always done with "return".

That rather assumes you're writing code to be reusable in some way. If expediency is more important than code then its fine to write things out from within functions.

planetsim

learn that the output of a function is always done with "return".

Not always there is an exception when only referencing the variable rather than copying it or creating it within the function

Weedpacket

Originally posted by axo
learn that the output of a function is always done with "return".

In a purely-functional language; PHP is not a purely-functional language.

BuzzLY

Yeah... PHP is not pure... it's a mangy mutt, and it will echo and print anywhere it wants. Just make sure it doesn't echo on the carpet -- we just had them shampooed. :p

axo

uh oh - i've trodden on some vespiary 😃 😃 😃

i agree with all of you that this is no "must" rule. but if i didn't say that it's a "must" rule, everyone would have thought about it as a "forgettable, optional" rule. at least i have woken you up 😃

if anyone of you likes it better to echo within functions - please feel free to do so. me myself, i don't like it, and i try to avoid it on every possible opportunity.

In a purely-functional language; PHP is not a purely-functional language.

you are right. unfortunately there are too few people who get to know at least one functional language. sure - functional languages have also disadvantages, but this fixation is one of the great advantages of it (and i have never seen any code that short and that cryptic 😃 😃 - so that's the second advantage: you don't need to hide the code, because after two weeks, nobody understands it anymore 😃 😃 ).

and nobody agrees with me at least at the point that echoing within functions is not elegant?

Weedpacket

Oh well; just remember that this is supposed to be real life, not a CS classroom. Generating your output as soon as you have it to generate and then immediately forgetting about it makes for a faster site with a smaller memory footprint. And that's a lot more important than some "rule" someone picked up somewhere.

Besides; PHP treats the entire script as a single function. It used to be referred to internally as main().

I'm going back to sleep, now.

xanas3712

on php-side, you load the form.tpl in with fopen, fread, fclose e.g. in a string variable $template. then create an array
PHP:
$trans = array(
 '{myPlaceHolder}' => "myPlaceHolderContent",
 '{mySecondPlaceHolder}' => 'mySecondPlaceHolderContent',
);
and when you're done with that you replace the placeholders with the php-generated content.
PHP:
return strtr($template,$trans);
by this, you separate html from php.

Hey, axo.. this sounds really interesting to me, but I've never seen this before.. could you either give a more specific example somewhere else or post one? I am formatting my html with echo statements inside functions, but this sounds a lot nicer however, I don't know where to start as far as doing it....

Just for an example of my ugly (I'm a beginner give me a break 🙂 )coding...

function processbody()
    {
        require "config.php";
        echo "\n".$t2."<TR><TD bgcolor='black' align='left' valign='top' width='15%'>".
             "\n".$t3."<table height='100%' width='100%' align='left' bgcolor='green' cellspacing='1'>".
             "\n".$t3."    <TR><TD bgcolor='black' align='center' valign='top'> Menu </TD></TR>".
             "\n".$t3."</table>".
             "\n".$t2."</TD>";

    echo "\n".$t2."<TD bgcolor='black' align='center' valign='top' width='70%'>".  
         "\n".$t3."<table height='130' width='100%' align='center' bgcolor='green' cellspacing='1'>".
         "\n".$t3."    <TR><TD bgcolor='black' align='center'><img src='logo.jpg'></TD></TR>".
         "\n".$t3."</table><left><h2>MatrixCentral News</h2></left>";
    $query = "SELECT * FROM news ORDER BY date DESC"; 
    mysql_connect($host,$user,$pass);
    mysql_select_db($dbname);
    $result = mysql_query($query);             
    while($row = mysql_fetch_assoc($result))
    {
        $date = date("l dS of F Y h:i:s A", $row['date']);
        echo "\n".$t3."<table width='100%' align='center' cellpadding='0' cellspacing='1' bgcolor='green'>".
             "\n".$t3."    <TR><TD width='50%' bgcolor='black' align='left' valign='center'>".$row['title']."</TD>".
             "\n".$t3."    <TD bgcolor='black' align='right' valign='center'>".$date." - ".$row['author']."</TD></TR>".
             "\n".$t3."</table>";
        echo "\n".$t3."<table width='100%' height='100' align='center' cellspacing='1' bgcolor='green'>".
             "\n".$t3."    <TR><TD bgcolor='black' align='left' valign='top'>".$row['news']."</TD></TR>".
             "\n".$t3."</table><BR><BR>";
    }
    echo "\n".$t3."<form action='".$PHP_SELF."' method='GET'>".
         "\n".$t3."    <input type='submit' value='Update' name='submit' style='background-color:black; color:lime;'>".
         "\n".$t3."</form>";     
    echo "\n".$t2."</TD>";                      
    echo "\n".$t2."<TD bgcolor='black' align='right' valign='top' width='15%'>".
         "\n".$t3."<table height='100%' width='100%' align='right' bgcolor='green' cellspacing='1'>".
         "\n".$t3."    <TR><TD bgcolor='black' align='center' valign='top'> Menu </TD></TR>".
         "\n".$t3."</table>".
         "\n".$t2."</TD></TR>";    
}

My screen res is high so the wrapping killed the formatting. I promise in quanta it's not nearly so ugly.

xanas3712

Still looking for more info on this, but I've converted to something similar, but it doesn't involve and array which I imagine allows for more flexibility...

$file = 'html.tpl';
$tpl = fopen($file,'r');
$data = fread($tpl,filesize($file));
fclose($tpl);
clearstatcache();    

$data = processheader($data);
$data = processbody($data);
echo $data;

And internal to the functions I'm just doing this....

$data = preg_replace('/{news_info}/',$news,$data);
        return $data;

The reason I am looking for something better, is that I still have one structure setup with a large amount of html code set up in it, because it is a loop, and I don't know how I would do that within this type of structure since I would have to replicate parts of the html.

I hope I'm not hijacking the thread asking this, but it seems to fit the original topic to ask for more specifics, if you want I'll edit these posts and move the real thing to the coding help forum instead.