|
|
Posted by Kye on 10/20/07 00:09
Thankyou for the critique. I have replied throughout.
> 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.
Ths is a very good point. I will have to make sure that I get into the habit
of using commenting to ensure that processes are remembered. Thankyou.
>> $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()
Its actually line 87, but you were only provided with the above information
so I understand that you can only comment on this information. The database
connection etc are all defined in the earlier protions of the page. This
chunk is my work so I was interected in learning what to fix in this chunk.
>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.
auto-prepend??? I am sorry but you have lost me. How do you mean about not
trying to find out what the error was? I thought that it was bad to leave
debug code in use after test mode was done,
>> 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,
I am afraid that you have lost me. What should have been done here instead?
> 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.
That one I knew was a bad thing but I am yet to learn how to validate input.
Is there any advise you could share on common pitfalls?
> 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,
I understand that you all have real jobs and that the constant bombardment
from low-end plebs is a real pain in the arse for you. Especially with the
number of burrow-sphincters out there who seem to expect you to pat them on
the back and tell them how great they are. I am simply gratified that you
spared what time you could to look at what code you had time so that you
could spend more time not making money to help a person start to learn.
> 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).
RTM 1-5 will do again. Hopefully now I will have a clearer understanding of
it having tried to use the syntax to actually do something with it. Thanks
for the help again.
--
Yours Sincerely
Kye
Navigation:
[Reply to this message]
|