|
|
Posted by C. (http://symcbean.blogspot.com/) on 10/19/07 22:13
On 19 Oct, 12:28, "Kye" <desca...@hotmail.com> wrote:
> As a real pleb with the below being my first actual "real" script written
> from scratch, could anybody please constructively criticise what should be
> changed???
>
> Thanks again for everybody's help
>
> --
> Yours Sincerely
> Kye
>
> ---START CODE---
>
> <?php
OK Kye, you asked for it....
There are no comments - I shouldn't have to reverse engineer you're
code to work out what its supposed to do and neither should you - the
very first line after <?php should be a comment explaining what the
program is supposed to do. Most folk also keep some rough versioning
info here too.
> $category = "SELECT * FROM `links_categories` ORDER BY category ASC LIMIT
> 0 , 30" or die('Error, query failed');
Variable assignments always work - why are you planning for a scenario
where it doesn't? (or die...). Most serious programmers would work to
a style guide which cover SQL as well as PHP. Your variable names seem
sensible-ish so far - personally I usually insist on database name
prefixing and using some formatting with SQL statements, e.g.
$get_categories = "SELECT *
FROM maindb.links_categories
ORDER BY category
LIMIT 0,30";
This makes it easier to read and (for example) insert things like
"WHERE user='" . $_SESSION['current_user'] . "'"
You've used no modularity at all within your code - you haven't even
divided it into modes of operation, although it does seem to have sort
of a preparation stage and sort of a presentation stage. It's much too
long - learn how to use functions (and classes) also think about
putting common code into 'include' files.
> $catlist = mysql_query($category) or die('Error, query failed');
By line 2 your code has failed - did you really write the rest of the
code above without even testing this? The reason it failed is because
you did not issue a mysql_connect() unless it was via an auto-prepend
(this is very bad practice). When an error occurs, you make the
program bomb out without even trying to find out what the error was.
> echo "<form method=\"post\" action=\"{$_SERVER['PHP_SELF']}\">";
line 3, and you've not yet got all the data for the page but you've
started writing the page, from here on we have a mish-mash of the
application logic and its presentation. And your HTML sucks big time.
You are also writing an unvalidated input variable
($_SERVER['PHP_SELF']) straght into your output thereby leaving you're
code wide open to XSS atacks.
3 Lines in and I'm afraid your code already looks awful. Like the
other guys, I can't bring myself to review all of this - hopefully now
you have some more sympathy with their position,
If you've never written a program before then its a good start. And I
wouldn't like to discourage you from learning to program - but you've
still got a very long way to go, go back and RTFM (chapters 1-5).
Good luck
C.
[Back to original message]
|