Feedback?

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---

$category = "SELECT * FROM `links_categories` ORDER BY category ASC LIMIT
0 , 30" or die('Error, query failed');
$catlist = mysql_query($category) or die('Error, query failed');
echo "

align=left>$row[1] "; switch ($row[0]){ case "1": echo ''; break; case "2": echo ''; break; case "3": echo ''; break; case "4": echo ''; break; case "5": echo ''; break; default: echo "Unrated"; break; } echo "

"; // height='12'>Click for Description class='long'> border='0' width='12' height='12'> echo "$row[3]

Added to Database :"; list($year,$month,$day)=split("-",$row[4]); $display_date=date(" F d, Y",mktime(0,0,0,$month,$day,$year)); echo $display_date; echo "

Re: Feedback?

On Fri, 19 Oct 2007 11:28:09 +0000, Kye 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

well, does it at least do what you want it to do? otherwise, I don't see the point in giving "constructive criticism" to code that does what it's supposed to do.

Re: Feedback?

> well, does it at least do what you want it to do? otherwise, I don't see
> the point in giving "constructive
> criticism" to code that does what it's supposed to do.

Yes it does do precisely what I wrote it to do, but it feels very
cumbersome. I am sure that it is also riddled with glitches, but I do not
have the knowledge to see what I should improve as of yet.

--
Yours Sincerely
Kye

Re: Feedback?

"Kye" wrote in message
news:dx0Si.1797$CN4.915@news-server.bigpond.net.au...
> 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???

yes...don't be a fucking dweeb! we spend our days cleaning up bad code. what
makes you think we'd want to look at more?

Re: Feedback?

"Henri" wrote in message
news:4718a11b$0$12795$426a74cc@news.free.fr...
> On Fri, 19 Oct 2007 11:28:09 +0000, Kye 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
>
> well, does it at least do what you want it to do? otherwise, I don't see
> the point in giving "constructive criticism" to code that does what it's
> supposed to do.

so, you like driving your yugo? sorry, i like to go fast and in
style...without worrying about bursting into flames. you obviously are still
in the noobish category as well.

Re: Feedback?

On Fri, 19 Oct 2007 08:55:22 -0500, Steve wrote:

> "Henri" wrote in message
> news:4718a11b$0$12795$426a74cc@news.free.fr...
>> On Fri, 19 Oct 2007 11:28:09 +0000, Kye 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
>>
>> well, does it at least do what you want it to do? otherwise, I don't
>> see the point in giving "constructive criticism" to code that does what
>> it's supposed to do.
>
> so, you like driving your yugo? sorry, i like to go fast and in
> style...without worrying about bursting into flames. you obviously are
> still in the noobish category as well.

ya, right

Re: Feedback?

On 19 Oct, 12:28, "Kye" 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---
>
>

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 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 "

Re: Feedback?

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 > 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 "";
>
> 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

Re: Feedback?

On Fri, 19 Oct 2007 11:28:09 GMT, "Kye" 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

Couple things that I learned along the way, and some of it from the helpful ppl
of this group.

1. Use functions or other code to 'figure out' data first, load it into
variables, then at the end of your page, start echo()ing out the HTML: Send
your $catlist to a function that will load the html into a var:

$list = myListFunc($catlist);
function myListFunc($list){
$html = "";
$html .= '';
$html .= " - Please Select A Category - \n";
while ($row = mysql_fetch_array($list, MYSQL_NUM)){
$html .= "{$row[1]}\n";
}
$html .= '';
$html .= "";
$html .= "";
return $html;
}


2. You can avoid a bunch of switch, if/else stuff if you can use the info you
allready have:
instead of:
switch ($row[0]){
case "1":
echo '';
break;
etc etc

you can do
echo '';

Re: Feedback?

Hi, just one hint:
This statement:

> $heading = mysql_query("SELECT links_categories.category,
> links_categories.cat_id FROM links_categories WHERE $_POST[dropdown] =
> links_categories.cat_id") or die('Error, Heading select failed');

might be a security risk because it could allow mysql injection into
your database.
I mean an attacker could change the value of $_POST[dropdown] to be
some valid
mysql code and thus manipulate your databse. To fix this you should
use
mysql_real_escape_string() on each variable that holds input from the
user.