Check for Palindromes - pls review my clunky code

Check for Palindromes - pls review my clunky code
0.0 0

#1

Hi - I know my code is really clunky (even compared to the “intermediate” solution :cry: ) but it’s working and that is a victory for me for now. However, it is failing ONE of the testcases, i.e. palindrome(“almostomla”) should return false

I cannot tell why. Can someone take a look at my code? Thank you in advance!

Your code so far

  // remove non-alpha-numeric characters
  str = str.replace(/[^0-9a-z]/gi, '');
  str = str.replace(/\s/g, ''); 
  
  //make everything lowercase
  str = str.toLowerCase();
  
  
  //build arrays of str - going forwards and backwards
  var fwdarray=[];
  fwdarray = str.split('');
  
  var bkwdarray=[];
  bkwdarray = str.split('');
  bkwdarray.reverse();
    

  
  //compare the 2 arrays to see if they are the same
    for (i=0; i < fwdarray.length; i++){
//         console.log(fwdarray);
//         console.log(bkwdarray);      
      if (fwdarray[i]===bkwdarray[i]){
        return true;
      }else 
        return false;
    }
  }

palindrome("eye");

Your browser information:

Your Browser User Agent is: Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/64.0.3282.186 Safari/537.36.

Link to the challenge:


#2

Inside the for loop:
a matches a then you return true.

You don’t check all the characters.


#3

It is pure coincidence your code passes the other tests. Your code really only checks if the first and last alphanumeric characters match. If they do, you return true and if they do not, you return false. The test cases just do not have the variety they should or you would be failing more.


#4

Ugh. Well that’s pretty disheartening. (Is there any way to give feedback on that so they can make the test-cases more robust?)

If I understand correctly, I can’t return anything in the IF statement b/c it will exit the loop at the first positive result.

To fix:
I tried setting a variable to “on/off” and then checking if that var was on or off after the loop but that resulted in the same problem. Or a similar one where the variable could get overwritten if the last letter of each array matched but none of the other letters matched, giving a false positive.

I even tried a WHILE loop but I didn’t know what to put in the loop itself, besides adding 1 to counter…?

I think I’m at an impasse at this point. (Google search for comparing 2 arrays turned up some more advanced (to my eyes) javascript solutions).

Send help .


#5

You can still use the for loop, but instead of returning true if the corresponding array characters are equal and false if not, why not first check if they are not equal and return false if so, otherwise do nothing. Then if the for loop completes, that means there was not a situation where the characters were not equal, so you can just return true.


#6

Thank you @randelldawson! It finally worked. Since the test-cases are not robust enough to have caught my bad code before, let me know if you see any problems with latest code (besides being very clunky still):

function palindrome(str) {
  // remove non-alpha-numeric characters
  str = str.replace(/[^0-9a-z]/gi, '');
  str = str.replace(/\s/g, ''); 
  
  //make everything lowercase
  str = str.toLowerCase();
  
  
  //build arrays of str - going forwards and backwards
  var fwdarray=[];
  fwdarray = str.split('');
  
  var bkwdarray=[];
  bkwdarray = str.split('');
  bkwdarray.reverse();
    

  
  //compare the 2 arrays to see if they are the same
    for (i=0; i < fwdarray.length-1; i++){
        console.log(fwdarray);
        console.log(bkwdarray);      
      if (fwdarray[i] != bkwdarray[i]){
        return false;
      }
    }
  return true;
  }

palindrome("eye");

#7

The above code has 3 extra lines of code which are not needed.
You can simply write:

var fwdarray = str.split('');
// The above line initialized (declared and assigned a value) in one step

var bkwarray = str.split('').reverse(); 
/* Since str.split('') returns an array, you can go ahead an reverse it and
 then the reversee array gets assigned to bkwarray */

There are other ways to reduce these 5 lines (such as seen below):

var fwdarray = str.split(''); // same reason as above
var bkwdarray = fwdarray.slice().reverse(); // slice makes copy of fwdarray, then reverses before assignment

The test cases for this challenge were robust enough to catch the problem in your code, because one test did fail. If you would have passed all the test cases with this code, then I would say they were not robust.


#8

Hi there,

Can I try to help you improve your code a little bit?

function palindrome(str) {
  // remove non-alpha-numeric characters
  str = str.replace(/[^0-9a-z]/gi, '');
  str = str.replace(/\s/g, '');

In the first regular expression you’re replacing everything from the str variable, except for alphanumeric values (spaces, dots, underscores, etc). That’s because of the ^ symbol means something like everything but…. So, the second assignment is redundant as it will never do anything.

Awesome site for RegEx study.

  //make everything lowercase
  str = str.toLowerCase();
  
  
  //build arrays of str - going forwards and backwards
  var fwdarray=[];
  fwdarray = str.split('');
  
  var bkwdarray=[];
  bkwdarray = str.split('');
  bkwdarray.reverse();

Just like randelldawson mentioned, you can make this 5 lines a little shorter, but it works anyway :slight_smile:

  //compare the 2 arrays to see if they are the same
    for (i=0; i < fwdarray.length-1; i++){
        console.log(fwdarray);
        console.log(bkwdarray);      
      if (fwdarray[i] != bkwdarray[i]){
        return false;
      }
    }
  return true;
  }

palindrome("eye");

On this part I think you wouldn’t need a loop and I think if you see the code in a week you will spend some time figuring out what it does.

Might be easier using some functions coming with the language. Array.prototype.join(), for example.

My approach would be something like this:

if( str != str.split('').reverse().join('') ) return false;

You compare your string (without splitting it in your previous code) against your string, but this time splitted, reversed and glued together again as a string. If it’s false, returns false. Else, triggers the last expression in your function, which returns true.

Hope it helped.

Happy coding!


#9

After str has been stripped of all non-alphanumeric characters, then @SpaniardDev’s solution can further be simplified with:

return str === str.split('').reverse().join('');

No need for an if statement, because the expression will be evaluated as true or false and then returned. Putting it all together would yield the following. You will notice I added the toLowerCase() function to go ahead and eliminate one extra line and allows me to drop the i modifier from the regular expression.

function palindrome(str) {
  str = str.toLowerCase().replace(/[^0-9a-z]/g, '');
  return str === str.split('').reverse().join('');
}