Page 1 of 1

More of a review request, but anywho...

Posted: Sun Apr 29, 2007 7:34 pm
by OasisGames
Ok, here's the deal, I've spent quite a while working on a forum / news site. I'm looking for reviews of both the code (you'll have to download it to check it out, it's rather long, too long for posting, also, I guarantee you're going to puke when you see it as I've never had an lessons in PHP, I know what I've learned from trial and error, and that's it, Link) and the result (Take a look here for my development platform or here for someone else's use of my CMS).
Now, I've only been doing PHP for the past few weeks, so the code will probably look ugly. I've tried to move all my style information into CSS sheets, which can be used as themes. On the code side, I'm looking for advice on how to improve things, specifically my BB code on my forum, my login system, and possibly a fix for the lack of file_put_contents for PHP 4. On the client side of things, I'd like some advice on improvement of my themes (the ones in the block on the first site are all mine).
Anyway, here's a basic overview of what the site is:
PHPwnage, as I've dubbed it, is a news system and forum. The news system is just simple SQL queries to collect from a table of articles. The forum was a little trickier. The point of the entire project was to be easily installable, which has proved to be true for the most part (it's a lot easier than installing phpNuke or phpBB... The agonizing hours of working that they both involved...) The system allows an administrator to add content in the site through a central "Admin Panel", such as news articles, new blocks, user pages, and forum boards. The forums are meant to be easy to use for members and include private messaging, BB code (which, of course, needs improvement to strengthen security), and indicators of what forums or topics contain new posts. Whether a topic has new posts is determined by a comma-separated list of user ID's which is cleared or added to. All of the site uses WWW-Authentication, which is something I'd like guidance in changing.

So, I think that about covers it. Download the installation, and take a look at the code (note: the config.php is made in the installer, 'fresh_install.php', look there if you need to know how that's all set up, or possibly run through the installation to get a better idea of how it works), take a look at the sample sites, and let me know what can be improved. I'd definitely like to fix up the code, but I really need some help with it.

:)

Posted: Mon Apr 30, 2007 1:32 pm
by Begby
This is pretty good for a first start... but there are a few basics you need to really get a handle on before you go further. Take care of these things then repost your code again.


1. There is no data checking anywhere. This code is wide open to sql injection. Read up on mysql_real_escape_string() and check out the security forum.

2. There is no proper indenting in your code, nor any code blocks. I want to claw out my eyeballs when reading your source.
- Always use indents on if statements or any other construct that uses {}.
- Put blank lines after and before sections to group things together along with comment blocks (its ok if this makes your code 10 times as long, readability is paramount)
- Although your comments are good, could probably be a bit more informative. i.e. WTF is the color_cookie.php file for?
- Make your SQL queries more readable, in general you should never have to scroll a page of code horizontally to read what you have, its encouraged to indent your SQL statements and make them multi line.
- Select a way that you want to indent your SQL and code, then be consistent with it.

3. You can update more than one thing with a query. You can do the following 'UPDATE info SET name=$_POST['name'], url = $_POST['url'] etc. all in one mysql query. I noticed you doing this in other spos, but not on the everywhere on the admin.php page. Although it looks more readable using separate queries, its a bad idea as it causes way more queries than necessary.

4. Turn on all errors including warnings and notices, then make sure and check all of your arrays to make sure the index exists before accessig it by checking with isset()

5. Although not required, you may want to consider adopting an MVC approach or at a minimum using a templating engine such as Smarty. You might want to hold off on MVC until you get a handle on things, but some sort of template approach will make your app even more reusable / pluggable and prevent you from having ten thousand echos/prints scattered throughout your code. Seperateing logic from the view is a beautiful thing.


To reiterate, the most important thing is that your code is readable. In its current state its going to be difficult for people to assist you.