Can someone suggest simplification for the code?

Question

public boolean isPalindrome()
{

    Stack myStack = new Stack();
    for(Node current = head; current!=null; current = current.next)
    {
        if(!myStack.isEmpty())
        {
            if(myStack.peek()==current.data)
            {
                myStack.pop();
            }else if(current.next!=null&&myStack.peek()==current.next.data)
            {
                continue;
            }
            else
            {
                myStack.push(current.data);
            }
        }else
        {

            myStack.push(current.data);
        }   

    }

    return myStack.isEmpty();
}

What I am doing here is using a stack to check whether a linked list is a palindrome. It works as expected only thing is I wanted to get rid of code duplication where the else condition has a push of the data onto the stack.

http://stackoverflow.com/questions/52002/how-to-check-if-the-given-string-is-palindrome — Raman Zhylich, Aug 01 '12 at 22:42

score 7 · Accepted Answer · answered Aug 01 '12 at 22:31

7

The algorithm is unfortunately not correct. For "abbaaa" it would report that that is a palindrome, although it isn't. Checking for palindromes without using the length is difficult.

abbaaa () -> push a
bbaaa (a) -> push b
baaa (ba) -> pop b
aaa (a) -> pop a
aa () -> push a
a (a) -> pop a
() -> palindrome

answered Aug 01 '12 at 22:31

Daniel Fischer

181,706
17
308
431

I had not considered this use case. Thanks for pointing it out. – Phoenix Aug 02 '12 at 00:40

score 2 · Answer 2 · answered Aug 01 '12 at 22:29

This is a somewhat classic problem. There are many ways to solve it in java. One of the easiest is this one:

boolean isPalindrome(String s) {
   for (int i=0, len=s.length(); i<len/2; i++) {
      if (s.charAt(i) != s.charAt(len-i-1)) return false;
   }
   return true;
}

(Strictly speaking, this is a rewrite rather than a refactoring; however, any rewrite that preserves method signatures can be seen as a refactoring... and it is certainly more efficient)

Justin · Answer 3 · 2012-08-01T22:37:25.667

1

If all you want to do is remove the code duplication between the two else conditions then remove them entirely.

public boolean isPalindrome()
{

    Stack myStack = new Stack();
    for(Node current = head; current!=null; current = current.next)
    {
        if(!myStack.isEmpty())
        {
            if(myStack.peek()==current.data)
            {
                myStack.pop();
                continue;
            }else if(current.next!=null&&myStack.peek()==current.next.data)
            {
                continue;
            }
        }                   
        myStack.push(current.data);             
    }

    return myStack.isEmpty();
}

edited Aug 01 '12 at 22:37

answered Aug 01 '12 at 22:26

Justin

471
4
16

Won't work the same as his posted code, it would push even when you pop or continue. – While-E Aug 01 '12 at 22:35
Fixed. Continue should restart the iteration so it wouldn't push. – Justin Aug 01 '12 at 22:38
Oh yeah, didn't think about that. Duh. – While-E Aug 01 '12 at 22:40

score 0 · Answer 4 · answered Aug 01 '12 at 22:37

0

A simplification of functionality;

boolean isPalinDrome(String testString) {
    return new StringBuffer(testString).reverse().toString().equals(testString);
}

answered Aug 01 '12 at 22:37

Reimeus

158,255
15
216
276

score 0 · Answer 5 · answered Aug 01 '12 at 22:38

This should provide same functionality without repeat. It is pointed out however that your algorithm doesn't seem to be correct.

public boolean isPalindrome()
{

    Stack myStack = new Stack();
    boolean doPush;
    for(Node current = head; current!=null; current = current.next)
    {
        doPush = true;
        if(!myStack.isEmpty())
        {
            if(myStack.peek()==current.data)
            {
                doPush = false;
                myStack.pop();
            }else if(current.next!=null&&myStack.peek()==current.next.data)
            {
                doPush = false;
                continue;
            }
        }   
        if(doPush){                
            myStack.push(current.data);  
        }           
    }

    return myStack.isEmpty();
}

Can someone suggest simplification for the code?

5 Answers5