Sug: Clean up PHP Code

[MattR]

I understand you have many developers now, but you must come up with a set of code style standards and follow them!

For instance:

if ($bbuserinfo[userid]==0) {
show_nopermission();
}

if ($userlist!="buddy")
{
$userlist="ignore";
$listtype = "Ignore";
}
else
{
$listtype = "Buddy";
}


Come on! Pick an if... style and keep it!

Also, is it THAT hard to put spaces around operators?


$var=$userlist."list";

$splitlist=explode(" ",$bbuserinfo[$var]);

while (list($key,$val)=each($splitlist)) {
if ($val!="") {
if ($userinfo=getuserinfo($val)) {
eval("\$listbits .= \"".gettemplate("listbit")."\";");
}
}
}

// draw cp nav bar
$cpnav[1]="#DFDFDF";
$cpnav[2]="#DFDFDF";
$cpnav[3]="#DFDFDF";
$cpnav[4]="#DFDFDF";

/////////////// an easier to read version /////////////////
$var = $userlist . "list";

$splitlist = explode(" ", $bbuserinfo[$var]);

while (list($key, $val) = each($splitlist)) {
if ($val != "") {
if ($userinfo = getuserinfo($val)) {
eval("\$listbits .= \"".gettemplate("listbit")."\";");
}
}
}

// draw cp nav bar
$cpnav[1] = "#DFDFDF";
$cpnav[2] = "#DFDFDF";
$cpnav[3] = "#DFDFDF";
$cpnav[4] = "#DFDFDF";


It takes roughly zero time at all to do this while coding and REALLY helps maintainability / hackability / READABILITY in the code. Also it helps improve the "professionalism" of the code; it looks, quite frankly, childish and unprofessional to have two distinct styles of if... statements RIGHT IN A ROW and also many wildly varying styles of operator/space placement.

[Mas*Mind]

despite vb being an awsome product (no doubt about that) I must agree with mattr that the code isn't very professional

readability: When I go through the code I often have to reposition some bits to understand it. Allthough Ed said the way the brackets are positioned in this thread (http://161.58.84.213/forum/showthread.php?s=&threadid=17050) to obtain readability is a matter of preference I still think this is far more readable:

while($post = $DB_site->fetch_array($posts))
{ if(++$counter%2 != 0)
{ $post[backcolor] = "#F1F1F1";
}
else
{ $post[backcolor] = "#DFDFDF";
}
$post[postdate] = vbdate($dateformat, $post[dateline]);
$post[posttime] = vbdate($timeformat, $post[dateline]);

// cut page text short if too long
if(strlen($post[pagetext]) > 100)
{ $spacepos = strpos($post[pagetext], " ", 100);
if ($spacepos != 0)
{ $post[pagetext] = substr($post[pagetext], 0, $spacepos)."...";
}
}
$post[pagetext] = nl2br(htmlspecialchars($post[pagetext]));

eval("\$postbits .= \"".gettemplate("threads_splitthreadbit")."\";");
}

then

while ($post=$DB_site->fetch_array($posts)) {
if (++$counter%2!=0) {
$post[backcolor]="#F1F1F1";
} else {
$post[backcolor]="#DFDFDF";
}
$post[postdate]=vbdate($dateformat,$post[dateline]);
$post[posttime]=vbdate($timeformat,$post[dateline]);

// cut page text short if too long
if (strlen($post[pagetext])>100) {
$spacepos=strpos($post[pagetext]," ",100);
if ($spacepos!=0) {
$post[pagetext]=substr($post[pagetext],0,$spacepos)."...";
}
}
$post[pagetext]=nl2br(htmlspecialchars($post[pagetext]));

eval("\$postbits .= \"".gettemplate("threads_splitthreadbit")."\";");
}

reusability: Many times I find code that's scattered around the place while they could be integrated into reusable functions (or do it the OOP way, allthough php isn't the best OO-language, it does support OO)

hardcodeness: There are still many things hardcoded in the code (words like 'buddy', 'ignore' etc..., html)

Don't get this wrong; I'm not saying vb is bad coded (you guys are doing an awsome job), I only say it could be better :) I beleive optimizing and cleaning up the code would make vb far more 'hackable', smaller (less code) and even more faster.

[MattR]

Lots of good points there Mas*Mind!

I code very similar to the way you do;

if( $var == 32 ) {

echo "stuff";

} else if( $var == 33 ) {

echo "I like bacon!";

} // end if

$var[ 4 ] = "joe";

$DB_site->query( "INSERT INTO bob VALUES( '" . addslashes( $joe ) . "' )" );


But I'm not asking for *my* standards ;) so much as I'm just requesting a standard. Even if it's done the way you provided on your first example
while
{ stuff
}
(which I don't particularly like) it's better than the mixed jumble we have now.

Also for a hardcodedness issue; make named constants for say usergroup IDs. Although an administrator is most likely ID X it would be good to replace
if( $usergroupid == x )

with
DEFINE( "ADMINISTRATOR", x );

if( $usergroupid == ADMINISTRATOR )

Now we still have the problem of guessing what ID is for administrator but if there IS a problem in the future (say the user accidentally removes the administrator group) you only have to change the x to y and it all works.

I will echo Mas' comment and say that while vB is a great product, there are some things that can be done to make it an even better one!! :D

[bira]

I made that comment as well to Kier once, in the hacks forum.

I wish they would stick to one standard, and the hackers would adopt it as well.

[Jake Bunce]

don't fix it if it ain't broke. :D

[etones]

Jakeman.. have you ever tried to make complex hack? To re the code?

If not... then u have no clue as to what actually is broken ;)

[Jake Bunce]

ahhhhh! code is evil! never write code! never hack code! only use visual html editors! etc

[John]

Thanks for the note. This is something that we (the developers) have discussed, and it is on our list of things to do to try and standardise the code.

John

[Steve Machol]

Originally posted by John
Thanks for the note. This is something that we (the developers) have discussed, and it is on our list of things to do to try and standardise the code.

John

John, I'm sorry but that's just not acceptable. You need to act much more defensively and deride these ingrates for even suggesting that there's something less than perfect about your code.

Haven't you learned anything from you main competition!

:D:D:D

[JohnM]

if (some_condition_is_met)
{
do_something();
}
else
{
do_something_else();
}

bad. too long, too many lines with just { or }. better to do:

if (some_condition_is_met) {
do_something();
} else {
do_something_else();
}

[etones]

true, too many line breaks.. however i like curly braces to match, looks neater + when tracking down error... much better!



{

{

{


}

}

}

if you get what i mean

[That's why you use [code] tags :) --Ed]

[etones]

yikes.. removed all my spaces.. my lovely spaces :(

the above code trinkit makes no sence now :)

[Mas*Mind]

Originally posted by JohnM
bad. too long, too many lines with just { or }.

True, but then write it like this:


if (some_condition_is_met)
{ do_something();
}
else
{ do_something_else();
}


The reason why people code like this, is because you easily can see where you have an unclosed bracket and which block belongs to where. It's especially usefull when you have lots of inner blocks.

This way you don't need comment like:

} // end if

[etones]

thats what i was trying to show.. cheers Mas*mind

[Mas*Mind]

another thing you might wanna consider:

1- Don't write functions that print from itself. Use functions like this:

function connectDB($host, $name, $pwd)
{ return(!@mysql_connect($host, $name, $pwd);
}

if(!connectDB("localhost", "root", "pwd");
error("Can't connect to database");


Ex: in $DB_Mysql.php there is automaticly echo-ed an error to the browser when there occurs an mysql error. This makes this class not very usefull when creating a hack that needs to display info on another page outside vbulletin. (because the errormsg won't be consistent with the layout of that page.)

2- Use a consequent naming system for variables and functions:

$bbUserInfo, connectToDB(): (begin lowercase, every new word uppercase)

Needless to say; Use very clear, understandable names for variables and functions.

3- Create reusable functions

Ex: When I went to vb's code to find out how difficult it would be to hack-in MIME compliant email I saw there were lots of mail() calls. This wouldn't be the case when there was one vbMail() function, so you only had to edit that function to integrate this.

It would be great if future versions will have an reusable API, so hacking would be much easier.

[Wayne Luke]

While I agree that coding standards are important, I feel that vBulletin goes further to adhere to any standard of the many scripts out there..

Sure there are a few inconsistancies and there are a few places where the code can be optimized and/or made more modular.

However I also feel strongly that people should get their own house in order before they criticize the habits of others...

Look at these two pieces of code:

if ($bbuserinfo[userid]==0) {
eval("standarderror(\"".gettemplate("error_nopermission_loggedout")."\");");
} else {
if($bbuserinfo[usergroupid] == 3) {
eval("standarderror(\"".gettemplate("error_nopermission_awaiting_moderation")."\");");
}
else {
eval("standarderror(\"".gettemplate("error_nopermission_loggedin")."\");");
}
}
exit;



if ($bbuserinfo[userid]==0) {
eval("standarderror(\"".gettemplate("error_nopermission_loggedout")."\");");
} elseif($bbuserinfo[usergroupid] == 3) {
eval("standarderror(\"".gettemplate("error_nopermission_awaiting_moderation")."\");");
} else {
eval("standarderror(\"".gettemplate("error_nopermission_loggedin")."\");");
}

exit;


You will find the second easier to read, more compact and using less resources than the first. They both do exactly the same thing.

[Mas*Mind]

However I also feel strongly that people should get their own house in order before they criticize the habits of others...

It's merely a good intended advice :rolleyes: