I know this can be better.

TobyRT

Hey there.

I'm going to open myself up to a world of pain, but it's the only way I'm going to learn!

I taught myself php from a book, online tutorials, and not a little help from some of you fine folks in here.

The problem is I know I'm still writing code too 'line by line' - functions and objects still baffle me.

What I'd appreciate here, if you'd oblige, is a critique of this code - it's fairly typical of the way I work. I'm pretty sure functions should be used here, but to be honest I don't know why - most functions I see simply appear to increase the amount of code that needs writing!

Background to this page:
It's part of a small-ish web-app I'm writing for a publisher to help with their workflow and bibliographic data storage. This is one page of about 8 that I'm re-writing/designing. It's a form containing a dozen or so fields pertaining to a given title. It will be operating in a controlled environment (i.e. an office with known hardware/software).

Basic operation:
User loads page. All fields are readonly/disabled
User clicks 'edit' button. readonly/disabled fields are toggled to enabled via jquery
For <select> dropdowns I display the current option in a <input> that is disabled. When the user clicks 'edit' it performs an ajax query to load and populate the <select>.
User makes edits as required. Clicks 'Save'.
Form is submitted, values written to database, page is reloaded with new values.

I'd also appreciate any advice you can offer on form validation - at the moment I'm only checking to see that the mandatory fields have a value. I'm reading around the subject at the moment and I'm totally confused, but I think that's a question for the 'newbie help' section of the site - but any tips in here would be gratefully received too.

Thanks in advance (and please be at least a little bit gentle!)

bradgrafelman

I realize this thread is a bit old, but I thought I'd offer my critiques anyway.

First and foremost, you should be more consistent with your whitespace usage. Some if() blocks are indented while others are not (which makes it hard to understand the logical flow at a glance).
Why do you call require_once('../includes/smhconn.php') twice (line #19 and #36)?

In this code:

		//if it hasn't been posted, I need the isbn to pull relevant data to populate the fields for updating
		if (!isset($_GET['isbn'])) {
		$isbn = $_POST['isbn'];
		} else {
		$isbn = $_GET['isbn'];
		}

what happens if $_POST['isbn'] isn't defined either?

This code:

		$pubdate = explode('/', $pubdate);
		list($pubday, $pubmonth, $pubyear) = $pubdate;
		$datestamp = array($pubyear, $pubmonth, $pubday);
		$pubdate = implode("-", $datestamp);

could be shortened a bit to something like:

		list($pubday, $pubmonth, $pubyear) = explode('/', $pubdate);
		$pubdate = "$pubyear-$pubmonth-$pubday";

Also, on the subject of validation, note that PHP supplies a function called [man]checkdate/man that you can use to verify that the date given is valid.

User-supplied data should never be placed directly into a SQL query, else your code will be vulnerable to SQL injection attacks and/or just plain SQL error messages. Instead, it must first be sanitized with a function such as [man]mysql_real_escape_string/man (for string data).
Although the 'or die()' technique is a very common quick 'n' dirty way to debug code while it is being developed, note that you should never leave such code in place in your production environment. Also note that you should not output the query or the SQL error message to the user - that type of information should be logged for later review.
As a matter of optimization/efficiency, you should avoid using 'SELECT *' and instead list the columns that you actually need data from (even if this includes all columns).
Why do you call [man]stripslashes/man on line #126 (and #238) when outputting $edition['edition_title'] ?
One thing I always try to do in all of my PHP projects is to separate PHP code from HTML. In other words, my .php files contain very little (if any) HTML code at all. It makes it a lot easier edit either the PHP or HTML code when they are separated from each other (e.g. through the use of templates).

TobyRT

Thanks Brad - I'll take a look at this now!!

vicodin

Anything you are putting into a DB shouls always, always always have mysql_real_escape_string();

For example you have:
$title = $_POST['title'];

It should be:

$title = mysql_real_escape_string($_POST['title']);

vicodin

"The problem is I know I'm still writing code too 'line by line' - functions and objects still baffle me."

You dont need to learn the object side or programing if your not going to be doing huge projects but you do need to learn how to make and use a function.

This makes your code 10000 times more readable, editable and organized. Besides that it allows for code reuse which will make the development time shorter... Take a day and teach your self this.

bradgrafelman

vicodin;10977771 wrote:
Anything you are putting into a DB shouls always, always always have mysql_real_escape_string();

Definitely not true! All data should be sanitized properly, yes; [man]mysql_real_escape_string/man is only appropriate for sanitizing string type data (as the function name suggests).

vicodin

You are correct, my bad on the other data types.