Is this a bad programming practice?

[jake66]

I've read around the web that using too many if's is bad, and to condense them.

I am not sure how to, with this:

if ($HTTP_GET_VARS['keywords'] =='query1'){
tep_redirect('mysite.com/query1.html');
exit();
}
if ($HTTP_GET_VARS['keywords'] =='query2'){
tep_redirect('mysite.com/query2.html');
exit();
}

..basically, I have dozens of these on my search box.
If the results are NOT being saved anywhere (simply used to FIND), do I need to sanitize them?

[andrewheiss]

I generally use a case switch, which looks at different options for a variable value rather than setting dozens of if statements. Look here for examples: [us3.php.net...]

[vincevincevince]

Provided that your search facility is well written (i.e. a database search with MySQL would use mysql_real_escape_string() on the query terms) then you do not need to sanitise before sending to the search function. On the other hand, you should seriously consider using either meta tags or robots.txt to ensure that search results pages are not crawled by search engines - making it impossible to use those results pages to poison your SERPS.

Take care where the search term is output again - don't just 'print $_GET[query]' as that can cause a lot XSS problems.

[jake66]

vincevincevince, If i use:

print $_GET[query]

like:

print strip_tags($_GET[query])

..is it still unsafe?

or what about:

$querystripped = preg_replace('/[^0-9]/i','',$QUERY_STRING);

(for numerical outputs)

you should seriously consider using either meta tags or robots.txt to ensure that search results pages are not crawled by search engines

Already in place. :) But there's been a lot of people posting their search results as of late, for terms I'd much rather have my static pages rank for.. rather than stuff coming off the on-site search engine. When people link to their search results, I get nothing.. because all robots are denied to the search page. At least (I figure), if I 301 my best terms I have a shooting chance at getting something out of it.

[PHP_Chimp]

Seeing as there is no database involved just stripping tags is easy additional security.

There is a good list of things to watch out for in $_GET hacking [webmasterworld.com]. Its mainly about SQL injection although there is some good old script and other tags getting thrown in as well as some cookie tampering. So a lot of good examples of the things people try to throw in.

Although back to your original question about if/elseif/else loops getting very long: the only other way is a switch statement with a lot of case's in there. If you dont have an else statement then switch is faster, however if you are using the final else statement then the if/elseif/else is faster than switch using default. At least on the server where I tested it last. So you may want to check the difference in speed (from memory the difference was about 0.2 seconds over 100000 iterations...so that is a VERY long list of keywords).

The only other thing is that you are using the old form of $HTTP_GET_VARS, the newer forward compatible version is just $_GET (quicker to type as well ;). Not that there is anything wrong with the older version, just if your host upgrades then it may no longer work.

BTW
$querystripped = preg_replace('/[^0-9]/i','',$QUERY_STRING);
You dont need to i...as 0-9 are not case sensitive ;)

[lammert]

I've read around the web that using too many if's is bad, and to condense them.

Really? What were the arguments used for this statement?