Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I just spent the whole night writing this little script for my search engine. Now i want to improve it.

Any ideas?

<?php 
echo "KEYWORDS : ".  $searchstring  = "the and";

$sentence = "Skype Home is unavailable at the moment. Check back later to see your news and alerts.  It's easy to start a conversation on Skype:  Choose a contact and start talking Call a phone or mobile from the dial pad, or send an SMS.";

echo "<br><br><b>ORIGINAL STRING</b><br><br>$sentence";


$kt=split(' ',$searchstring); //Breaking the string to array of words

$num=1;

while(list($key,$val)=each($kt)){
    // do not load sentence from session on first run!  

    if($num<=1){
    } else {
        $sentence  = $_SESSION['NewSentence'];
    }
    if($val<>" " and strlen($val) > 0){

        // make keyword bold and send entire sentence to session

        $_SESSION['NewSentence']    = eregi_replace ("$val", "<b>$val</b>", $sentence);
        // output sentence with added keyword

        echo "<br><br>$num bolding:  <b style='color:green;'>$val</b> <br><br>  ".$_SESSION['NewSentence'];

        $num++;
    }
}

// make all bold keywords to red`

echo '<br><br><b>FINAL STRING</b><br><br>';
echo  $_SESSION['NewSentence'] = eregi_replace ("<b>", "<b style='color:red'>", $_SESSION['NewSentence']);

//destroy  session
unset($_SESSION['NewSentence']);
session_unregister($_SESSION['NewSentence']);?>
share|improve this question

2 Answers 2

Use of deprecated functions

Both split() and eregi_replace() are deprecated since PHP 5.3. Don't use them.

Use preg_replace() instead of eregi_replace() and explode() instead of split() Other alternatives are str_replace() and preg_split().

Unexpected results

When the search string is lowercase, all replaced words will be lowercase too. The case of the original word is not preserved.

Unused variables

The $key variable is assigned but never used. In fact, the entire while loop is odd. Use a foreach loop instead, like so:

foreach($kt as $val) {
    ...

Missing function

The code should be in a function. The function would require two parameters, the string and an array of words to highlight, and return the result string. The function should not echo anything (but I presume the echo statements are debugging things).

Old HTML

Although valid, the use of <b> tags is outdated. It describes formatting in a document layout, without specifying the need for the format. You should use html for layout and css for styling. Then you can replace the <b> with a <span class="highlight> (or any other sensible class name).

share|improve this answer

Unused if statements

You have a if($num<=1) statment that is empty, while your using else. Reverse the statement like this.

if ($num > 1) {
    $sentence  = $_SESSION['NewSentence'];
}

Ambiguous use of $_SESSION

You should not use $_SESSION as a storage unless you are going to use it in another session (read: new page load/refresh). At the end of your script you're destroying the session, if that's your intention use variables as placeholders and leave $_SESSION alone.

Hard to read code

You should always aim to make the code as readable as possible. Take for example look at PSR-1 and PSR-2 to learn a coding standard.

share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.