Newbie PHP programming looking for constructive criticism

[chrziz]

I've been coding simple PHP scripts for a few years now but I know my coding techniques probably aren't the most effective as I am all self-taught.

 

I wrote this simple script to get input from a text field, search if the first word is a hard coded "command", then search the respective provider defined by the "command" with the text following the command. The script is short and pretty self explanatory.

 

Can anyone offer any constructive criticism as making the script more effective thus faster (as far a script loading time)? Any things I'm doing right, wrong, etc. Any tips would be highly appreciated so I can write better quality scripts in the future. Thanks!

 

http://rafb.net/p/bynfAk93.html

[trq]

One thing you could do simply would be to remove the unneed loop. So....

 

$words = explode(" ",$input);	
// First word of array is the command
$command = $words[0];	
// Following text is the query
$num_words = count($words);

// Add query words to one string separated by + signs
for ($i=1;$i<=$num_words-1;$i++) {
		if ($i == $num_words-1) {
			$query .= $words[$i];
		} else {
			$query .= $words[$i] . "+";
		}
}

 

would become....

 

$words = explode(" ",$input);	
// First word of array is the command
$command = array_shift($words);	
// Following text is the query
$query = implode('+', $words);

[trq]

Also, most of your code should be within an if condition. ie; Instead of simply...

 

$input = stripslashes($_POST['q']);

 

You should use....

 

if (isset($_POST['q'])) {
  $input = stripslashes($_POST['q']);

  // rest of code.

}

[chrziz]

Thanks a lot for the input!

 

Is the reason for adding the if conditions to prevent errors?

[trq]

Is the reason for adding the if conditions to prevent errors?

 

Indeed. If $_POST['q'] is not set the line you have will generate a warning.

 

Of course, if you deloped all your code with error reporting set to E_ALL (which you should always do) you would have seen this warning.

[chrziz]

I modified the script with your changes, it works perfectly and faster. Nice job!

[trq]

Actually, if you remove the loop using the code I provided you will also need to change this section....

 

// If command is found, search the query with the selected provider	
if ($search_path) {
	header("Location: " . $search_path . $query);
// If no command, search the whole entry in Google
} else {
	header("Location: http://www.google.com/search?hl=en&btnG=Google+Search&aq=f&oq=&q=" . $input);	
}

 

to....

 

// If command is found, search the query with the selected provider	
if ($search_path) {
header("Location: " . $search_path . $query);
// If no command, search the whole entry in Google
} else {
header("Location: http://www.google.com/search?hl=en&btnG=Google+Search&aq=f&oq=&q=" . $query . '+' . $input);	
}

[chrziz]

I just noticed that after I checked the debugging outputs and actually ran the script.

 

I will research these techniques you've showed me and learn how to implement them in all my future scripts.

 

You've been very knowledgeable on this topic. Thanks for all the help!