Security and efficiency

[Kudose]

This code below converts a users input and colors they select into random colors ouput in UBB format.

My question is, is there any way to tweak this to make it flow better, more secure, etc. I want to eventually donate the completed source code to a friend so I want it to be pretty secure.

Thanks 🙂

//start the every letter color script
function everyletter($message, $color){
//if server is not running PHP 5.0.0 or greater you need this next part
if(!function_exists('str_split')) { 
function str_split($string) { 
$chars = array(); 
for($i=0;$i<strlen($string);$i++) { 
$chars[] = $string{$i}; 
} 
return $chars; 
} 
} 
//end PHP5 function replication
//*****************************
//start conversion and display
$split_message = str_split($message);
$num = count($split_message);
$last_color = "blue";
for($i=0; $i<$num; $i++){
$select_color = $color[array_rand($color)];
while($select_color == $last_color){ $select_color = $color[array_rand($color)]; } //make sure colors dont repeat
if($split_message[$i] == " "){ //if message is a space
echo " ";
$last_color = $select_color;
} else { // other half
if($select_color == "black"){ //if random is black
$split_message[$i] = stripslashes($split_message[$i]);
if($split_message[$i] !== ""){ 
echo $split_message[$i];
$last_color = $select_color;
	} //end if
} else { //else not black
$split_message[$i] = stripslashes($split_message[$i]);
if($split_message[$i] !== ""){ 
echo "[".$select_color."]".$split_message[$i]."[/".$select_color."]";
$last_color = $select_color;
} //end if blank 
			}// end if
		}//end if

}// end for message
echo "\n\nPowered by Spechal.com";
} // end everyletter function

[drawmack]

I don't see any security issues, however I see something that could be a speed issue with long strings. You should call stripslashes once before you split the string instead of calling it on every individual character. That will save the repeated calls to stripslashes and the null string if tests. All in all probably about 5 clock ticks per character savings which adds up pretty quickly.

[Weedpacket]

I don't know if preg_split() would be a faster way of emulating str_split() than the loop, but if drawmack's suggestion of moving stripslashes() out of the loop were to be followed, the issue would become moot anyway. Slash stripping is the only change made to the string. Once it's done the string is only ever read from (one character at a time). And of course you already know you can read a character of a string using the $string{$offset} syntax. And poof! the need for str_split() disappears in a puff of logic!

[Kudose]

Ypu mean something like this:

//take out str_replace
stripslashes($message);
strip_tags($message);
$num = strlen($message);
for($i=0; $i<$num $i++){
$select_color = $color[array_rand($color)]; 
while($select_color == $last_color){ $select_color = $color[array_rand($color)]; } //make sure colors dont repeat 

//place all my if's and add to the strig like this

$message{$i} = "[".select_color."]".$message{$i}."[/".$select_color."]";

$last_color = $select_color;

}//end for
echo $message;

Whould that be something like you guys are trying to explain?

[drawmack]

yup that'll do it, might make it marginally faster to strip the tags first though.