Palindrome checker in JavaScript

Question

I've built a Palindrome checker in JavaScript. It's fairly simple.

I'm still learning JS and am looking to learn.

So I would love hear ideas on how to improve the JavaScript in this. Like more efficient ways to resolve the task of checking for Palindrome.

Source code is below. Link to CodePen is her: http://codepen.io/MarkBuskbjerg/pen/JWMWwN?editors=1010

HTML:

<div class="cover">
    <div class="container header">
        <div class="row row-margin">
            <div class="col-sm-12">
                <h1 class="text-center">Palindrome checker</h1>
            </div>
            <div class="col-sm-12">
                <p class="text-center text-uppercase">A nut for a jar of tuna</p>
            </div>
        </div>
    </div>
    <div class="container">
        <div class="row row-margin">
            <div class="col-md-2">

            </div>
            <div class="col-md-8">
                <textarea class="col-md-12" id="inputPalindrome" rows="5">Borrow or rob?</textarea>
            </div>
        </div>
        <div class="row row-margin">
            <div class="col-sm-3">

            </div>
            <button id="checkPalindrome" class="btn btn-default btn-lg btn-block col-sm-6 col-sm-offset-4" type="submit">Check palindrome</button>
        </div>
    </div>
    <div class="container">
        <div class="row row-margin">
            <div class="col-md-2">
            </div>
            <div class="col-md-8 text-center">
                <div id="notification" class="alert alert-info">Palindrome has not been checked yet</div>
        </div>
    </div>
</div>

CSS:

.cover {
  height: 100vh;
}

.row-margin {
  margin: 4vh auto;
}

body {
  background: #FF4E50;
  background: -webkit-linear-gradient(135deg, #FF4E50, #F9D423);
  background: linear-gradient(135deg, #FF4E50, #F9D423);
}

.header {
  color: #fff;
}

h1 {
  font-family: "Pacifico";
}

.btn-default {
  background-color: black;
  color: #fff;
  font-weight: bold;
  text-transform: uppercase;
 }

JavaScript:

var checkButton = document.getElementById("checkPalindrome");

function isPalindrome(str) {
    str = str.toLowerCase().replace(/[^a-z0123456789]+/g,"");
    var reversedStr = str.split("").reverse().join("");
    if (str == reversedStr) {
        return true
    } 
    return false
}

checkButton.addEventListener("click", function() {
    var palindromeInput = document.getElementById("inputPalindrome").value;
    var palindromeReturn = isPalindrome(palindromeInput);
    if(palindromeReturn === true) {
        document.getElementById("notification").innerHTML = "Yay! You've got yourself a palindrome";
        document.getElementById("notification").className = "alert alert-success";
    } else {
        document.getElementById("notification").innerHTML = "Nay! Ain't no palindrome";
        document.getElementById("notification").className = "alert alert-danger";
    }
});

Link to CodePen with the Palindrome Checker http://codepen.io/MarkBuskbjerg/pen/JWMWwN

Answer 1

RegEx

In character class, 0123456789 can be written as range 0-9. Although, this has no effect on working of RegEx, it saves some keystrokes and looks consistent with alphabets range a-z.

toLowerCase after replace

First remove the special characters and then convert the string to lower case. This, in my opinion will run fast than other way round as the number of characters to work on are reduced.(Not Tested)

When doing this, don't forget to add i flag on the RegEx.

str.replace(/[^a-z0-9]+/gi, "").toLowerCase();

return boolean;

if (str == reversedStr) {
    return true
} 
return false

can be written as return str == reversedStr. It is also recommended to use strict equality operator.

return str === reversedStr;

See Which equals operator (== vs ===) should be used in JavaScript comparisons?

Caching DOM element reference

In the click handler of button, #notification is referenced four times. All times, it is read again from DOM. This can be slower. The element can be cached and used whenever required.

var notification = document.getElementById('notification');
...
...
notification.innerHTML = 'Hello World!';

---

Complete Code

Updated after suggestions from Ismael Miguel

// After DOM is completely loaded
document.addEventListener("DOMContentLoaded", function() {
    "use strict";

    // Cache
    var palindromeInput = document.getElementById("inputPalindrome");
    var notification = document.getElementById("notification");

    function isPalindrome(str) {
        str = str.replace(/[^a-z0-9]+/gi, "").toLowerCase();
        return str.split("").reverse().join("") === str;
    }

    document.getElementById("checkPalindrome")
        .addEventListener("click", function() {

            if (isPalindrome(palindromeInput.value)) {
                notification.innerHTML = "Yay! You've got yourself a palindrome";
                notification.className = "alert alert-success";
            } else {
                notification.innerHTML = "Nay! Ain't no palindrome";
                notification.className = "alert alert-danger";
            }
        });
});

Answer 2

I'll just add a couple of observations to Tushar's existing answer, as that's a very solid teeing-off point.

Remove global scope

At present, your code is all going into the global scope of the document. If you happen to use another script on the page, it's possible (although in this case likely) that something else may also define an isPalindrome function. By using an IIFE you can eliminate the risk of this happening:

(function () {
    function isPalindrome (str) {
        // ...
    }

    document.getElementById("checkPalindrome")
        // ...
}());

A bonus of doing this, as pointed out by Ismael Miguel in comments, is that you can then extract the 'use strict' into the outside function, preventing the need to repeat it.

Consistent quotes

You're using a mish-mash of single quotes and double quotes; for example you have getElementById("checkPalindrome") but then getElementById('notification') later on. More notably, you set innerHTML = 'Yay! You\'ve in the true block, but then innerHTML = "Nay! Ain't in the false block.

Generally speaking I'd recommend using single quotes 99% of the time but using double quotes if the string contains another single quote.

Comments should make sense

Simple enough one here - what does "value is much clear" mean? In fairness, this is probably just a comment directed at you, from Tushar, but still... it's not the kind of comment you'd write in your production code. I'm just mentioning this for completeness sake more than anything else.

// value is much clear
var value = document.getElementById("inputPalindrome").value;

Unnecessary characters in regex

Another very minor one - these two regexes are identical:

/[^a-z0-9]+/gi
/[^a-z0-9]/gi

Because you have the g modifier (global - match every occurrence), you don't need the + symbol in the regex (one or more occurrences).

Combine multiple var statements

This one is purely down to your preference, but I generally feel that combining multiple var statements into one block reduces noise in the code and makes it easier to scan.

var value = document.getElementById('inputPalindrome').value,
    notification = document.getElementById('notification');

Name your closures

Something that you may not know is that in your anonymous click handler function, you can actually give it a function name:

.addEventListener("click", function checkPalindrome () {

There's one significant benefit of doing this: when you're debugging something in the console, if you name your functions then their names will show up in the stack trace.

Extract closures if they contain significant logic

This is again more of a personal preference; your checkPalindrome function is doing a fair bit of work, so rather than just declaring it inline I'd personally declare it separately then just pass the reference to it into the .addEventListener call.

function checkPalindrome () {
    'use strict';
    // ...
}

document.getElementById('checkPalindrome')
    .addEventListener('click', checkPalindrome);

Another nice bonus is that it removes a level of indentation from your code; generally speaking, the more indents you have, the harder it is for someone else (or you in two months time) to pick up the code and understand.

Hoist DOM lookups outside of the click handler

Another point raised by Ismael Miguel below; we can move the document.getElementById calls out of the click handler so they only get evaluated once. In this scenario it's a micro-optimisation but if you have handlers which can be called on many elements in quick succession, it's a very good habit to be in.

Complete Code

(function () {
    'use strict';

    var value = document.getElementById('inputPalindrome').value,
        notification = document.getElementById('notification');

    function isPalindrome (str) {
        str = str.replace(/[^a-z0-9]/gi, '').toLowerCase();
        return str.split('').reverse().join('') === str;
    }

    function checkPalindrome () {
        if (isPalindrome(value)) {
            notification.innerHTML = "Yay! You've got yourself a palindrome";
            notification.className = 'alert alert-success';
        } else {
            notification.innerHTML = "Nay! Ain't no palindrome";
            notification.className = 'alert alert-danger';
        }
    }

    document.getElementById('checkPalindrome')
        .addEventListener('click', checkPalindrome);
}());

Answer 3

I have two objections to the Javascript function as written, both as submitted and as rewritten by other commenters:

1. It is fairly difficult to inspect or test. All the steps are intertwined. If you had forgotten, for example, to handle upper-case, you might very well not have noticed in all that code.
2. All you end up with is one, not very useful function. How likely is it that in the future you will again need a function to determine is a string is a palindrome considering only down-cased alphanumerics?

My proposal is this:

const arrayFunctionOnString = (s, f) => f(s.split("")).join("");

const reverseString = s => arrayFunctionOnString(s, d => d.reverse());

const cleanString = s => s.toLowerCase().replace(/[^a-z0-9]/g, "");

const isPalindrome = s => s === reverseString(s);

const isLoosePalindrome = s => isPalindrome(cleanString(s));

Not only is each step easy to examine and test in isolation, but when you are done, you have a nonce function, isLoosePalindrome() for the problem at hand, one possibly useful function, isPalindrome(), and three new functions that you will definitely need again in the future.

Edit:

Or, if you got excited, you could write:

const compose = (f, g) => x => f(g(x));

const isLoosePalindrome = compose(isPalindrome, cleanString)

Answer 4

You need to remove whitespace characters as well.

Race Car is also a palindrome, but with your current code race car would be reversed to "rac ecar" and give a false negative.

You can get this functionality by adding the following code to your isPalindrome function.

var variableName = variableName.replace(/\s/g, ''); 

Answer 5

You can solve it without using regular expressions or reversing the string by just comparing the first and last characters (ignoring any characters that do not meet your requirements - i.e. non-alphanumeric characters) and then iterating inwards from both ends until you reach the middle of the string:

function isValidPalindromeCharacter( char ){
  return ( 'A' <= char && char <= 'Z' )
      || ( 'a' <= char && char <= 'z' )
      || ( '0' <= char && char <= '9' );
}

function isCharactersEqual( a, b ){
  return a.toLowerCase() == b.toLowerCase();
  // return (a.charCodeAt(0)|32) == (b.charCodeAt(0)|32);
}

function isPalindrome(str){
  var lower = 0,
      upper = str.length - 1;
  while ( true )
  {
    while ( lower < upper && !isValidPalindromeCharacter( str[lower] ) )
    {
      lower++;
    }
    while ( lower < upper && !isValidPalindromeCharacter( str[upper] ) )
    {
      upper--;
    }
    if ( lower >= upper )
    {
      return true;
    }
    if ( !isCharactersEqual( str[lower], str[upper] ) )
    {
      return false;
    }
    lower++;
    upper--;
  }
}

var palindrome   = document.getElementById("inputPalindrome");
var notification = document.getElementById('notification');

document.getElementById("checkPalindrome")
  .addEventListener("click", function() {
    'use strict';

    // value is much clear
    var value = palindrome.value;

    if (isPalindrome(value)) {
        notification.innerHTML = 'Yay! You\'ve got yourself a palindrome';
        notification.className = "alert alert-success";
    } else {
        notification.innerHTML = "Nay! Ain't no palindrome";
        notification.className = "alert alert-danger";
    }
  });

<div class="cover">
  <textarea id="inputPalindrome" rows="5">Borrow or rob?</textarea>
  <button id="checkPalindrome" type="button">Check palindrome</button>
  <div id="notification">Palindrome has not been checked yet</div>
</div>