FlyingWay;10983420 wrote:
but it's not all information in the table
Well, you have both a where clause and a limit clause in the query, so you may obviously select less than everything in the table. Specifically, if there are either rows where ihome != 1 or there are more than 3 rows.
$qdb = "WHERE (ihome='1')";
$result = $db->sql_query("SELECT * FROM ".$prefix."_stories WHERE (ihome='1') ORDER BY sid DESC limit 3");
# obviously executes the following query
"SELECT * FROM ".$prefix."_stories WHERE (ihome='1') ORDER BY sid DESC limit 3"
# but if you instead run this, you retrieve all rows in the table
"SELECT * FROM ".$prefix."_stories ORDER BY sid DESC"
On the other hand, since this is wrapped in a function, hopefully to do something specific, changing a query like this may yield undesireable results, such as if the returned rows are supposed to be updated, deleted or whatever.
You also have several "yikes" in your code.
function index() {
/* global data = bad! google "globals are bad"
* one example: http://c2.com/cgi/wiki?GlobalVariablesAreBad
* Also, $news_home is never used!
*/
global $home, $db, $news_home, $prefix;
/* while $db might be treated as a global, why not wrap database access in a singleton
* which provides no access to $db?
* This would also make it easier to change things if you one day switch storage
* systems etc.
*/
/* Here's a perfect example of why globals are bad!
* Suddenly $home has changed, not just here, but globally!!! I rearely use more than
* one exclamation mark, but I'm tempted to add "oneone" after too, just for good measure.
* Why should index() change $home on a global scope?
* What if another function, foo(), believes $home should be 2?
* What if another function, bar(), always uses the current value of $home.
* Then suddenly bar() does different things depending on wether you called index()
* or foo() just before bar(), but when you just look at the lines:
if ($something)
foo();
else
index();
bar();
* you really have no idea that bar() will do entirely different things depending on
* $something
* while:
if ($something)
{
foo(); # not making use of global $home, it just uses 2 inside the function
bar(2); # not making use of global $home, it gets this value as a function arg.
}
else
{
# same reasoning as above
index();
bar(1);
}
* Also, if index() always only deals with i in this context, why use a variable at all and
* not just type 1 where it's needed inside this function?
*/
$home = 1;
$qdb = "WHERE (ihome='1')";
# like all the others, $prefix doesn't need to be global
$result = $db->sql_query("SELECT * FROM ".$prefix."_stories $qdb ORDER BY sid DESC limit 3");
/* indent your code properly! the mess this while loop was in tells the eye nothing about
* where the block starts and ends
*/
while ($row = $db->sql_fetchrow($result))
{
# is used once, to be stored in $more, which is NEVER used after assignment
$s_sid = intval($row['sid']);
# is only used in the nested query. can be removed when you run one query using joins.
$topic = intval($row['category']);
# is only ever assigned to (albeit assigned the same value twice!"
$title = stripslashes($row['title']);
# is only ever used one, to assign its value to $text! why not do that right away?
$hometext = stripslashes($row['hometext']);
# is only ever assigned to!
$image = $row['image1'];
# is only ever used once. Why not put it as function argument to GetAdminName directly
$adminid = $row['adminid'];
$admin_name = GetAdminName($adminid);
# here you assign the value of $title to $title!
$title = "$title";
# $time has not yet been initialized!
# $date is never used again!
$date = $time;
/* but here it is, and then it's never used again!
* why not either ditch the above, or directly do $date = $row['time']
* but if it's a time, store it in $time, if it's a date, store it in $date
* but then also change the name of the column.
*/
$time = $row['time'];
/* Pretty much always when you run one query and then run another query while looping
* over the result set of the first query, you should instead use an INNER JOIN
* although sometimes a LEFT JOIN is appropriate
* Apart from that, the same reasoning holds here, if you want all data in the table
* remove the WHERE clause.
*/
$row = $db->sql_fetchrow($db->sql_query("SELECT topictext FROM ".$prefix."_topics WHERE topicid='$topic'"));
$topictext = $row[categorytext];
$more = "news_view_$s_sid.html";
$by = "اby: $admin_name";
# $text is never used!
$text = "$hometext ";
$tmpl_file = "ndex_box.html";
# why implode the contents of file() and not use file_get_contents()?
$thefile = implode("", file($tmpl_file));
/* this entire extra-indented block... */
$thefile = addslashes($thefile);
$thefile = "\$r_file=\"".$thefile."\";";
# eval!?
# seriously?
# why?
eval($thefile);
print $r_file;
/* could instead be written as */
print(addslashes($thefile));
/* at least assuming the contents of ndex_box.html doesn't contain php code,
* but if it's an html file, why should it? And what if someone, by accident or
* with malice adds harmful php statements to ndex_box.html?
*/
}
echo "<br />";
}
