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.

How can this be optimized?

$text = "reverseesrever"; // Reverse and esrever
$b = false;

for($i = 0; $i < strlen($text); $i++) {
    if($text[$i] !== $text[strlen($text) - 1 - $i]) {
        $b = true;
        break;
    }
}

echo ($b) ? "YES" : "NO"; // result: NO.
share|improve this question
add comment

3 Answers 3

up vote 5 down vote accepted

The title of the question is confusing. The code seem to check if the text is not the same if you read it backward. For example, it $b will be true for "hello" and false for "level". In other words, you're checking if some text is NOT a palindrome. I think you should change the title to better reflect what the code is doing.

Things to improve:

  • It's enough to iterate until strlen($text) / 2, no need to go all the way until the end
  • To avoid re-calculating the length of the string, it's better to calculate it once before the loop and reuse it
  • Wrap the whole thing in a function to make it easier to reuse and to test
  • The !== is unnecessary, a simple != would do

For example:

function isPalindrome($text) {
    $length = strlen($text);
    for ($i = 0; $i < $length / 2; $i++) {
        if ($text[$i] != $text[$length - 1 - $i]) {
            return false;
        }
    }
    return true;
}

function test_isPalindrome($text) {
    printf("isPalindrome(%s) -> %s\n", $text, isPalindrome($text) ? 'YES' : 'NO');
}

test_isPalindrome("reverseesrever");
test_isPalindrome("hello");
test_isPalindrome("aba");
test_isPalindrome("aa");
test_isPalindrome("ab");

Outputs:

isPalindrome(reverseesrever) -> YES
isPalindrome(hello) -> NO
isPalindrome(aba) -> YES
isPalindrome(aa) -> YES
isPalindrome(ab) -> NO
share|improve this answer
    
thank you so much! –  user3592852 May 1 at 14:58
    
@user3592852 You're welcome! Make sure to reload the page, I improved my answer a few minutes ago. –  janos May 1 at 14:59
1  
@janos You haven't included the most important part and that is the incorrect use of strlen function. –  user41403 May 1 at 15:19
    
Good point, I totally overlooked that. Thanks, I improved my answer. –  janos May 1 at 15:32
add comment
  1. You are using strlen method each cycle 2 times, that means that if you have a string which is long 100 characters, then you will use strlen 200 times!

    For cycle in PHP works like this:

    for (expr1, expr2, expr3){
    
    }
    
    • expr1 is evaluated once
    • expr2 is evaluated in the beginning of each iteration
    • expr3 is executed at the end of each iteration

    You should calculate the length of string before using it in for cycle.

    Read more about for cycle in PHP documentation.

  2. As mentioned in the other answer by @janos, you don't need to iterate over whole string, but you still need to know the length of whole string for this condition:

    if($text[$i] !== $text[strlen($text) - 1 - $i]) {
        $b = true;
        break;
    }
    

    Otherwise you will always get YES as result.

  3. You can use '' for wrapping your string instead of "". In the second case PHP interpreter does not need to check for variables inside the string. This does not have any noticable impact on performance.

  4. You are using !== but in this case it is sufficient to use !=, however, this does have any noticable impact on performance as well.

  5. If you are going to reuse the code, you might consider wrapping it into a function.

    I rewrote your code (no function included):

    $text = 'reverseesrever';
    $iterateLength = strlen($text) / 2;
    $length = strlen($text) - 1;
    
    $b = false;
    for($i = 0; $i < $iterateLength; $i++) {    
        if($text[$i] != $text[$length - $i]) {
            $b = true;
            break;
        }
    }
    
    echo ($b) ? 'YES' : 'NO';
    
share|improve this answer
    
thank you for your response! –  user3592852 May 2 at 3:42
add comment

I'm not sure if I'm missing the point of your function, but I feel it could be a lot shorter and more efficient.

I suggest using a simple strrev() to reverse the string, for comparison.

Try,

$string = 'reverseesrever';
echo ($string === strrev($string)) ? 'YES' : 'NO';

I've used the same strings as a previous answer, and I get the same response.

"reverseesrever" -> YES
"hello" -> NO
"aba" -> YES
"aa" -> YES
"ab" -> NO

This would cut out iterations and multiple calls to strlen.

share|improve this answer
add comment

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.