8
\$\begingroup\$

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

\$\endgroup\$
12
\$\begingroup\$

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";
            }
        });
});
\$\endgroup\$
  • 2
    \$\begingroup\$ if (isValuePalindrome === true) can just be if(isPalindrome(value)) \$\endgroup\$ – Dan Cantir Mar 23 '17 at 12:27
  • \$\begingroup\$ Nice. Thanks a bunch for these tips and great explanations. Great help Tushar \$\endgroup\$ – Mark Buskbjerg Mar 23 '17 at 12:28
  • \$\begingroup\$ var notification = document.getElementById('notification'); isnt actually needed at all. In chrome at least, all ids are already defined as global variables. \$\endgroup\$ – Kaspars Mar 23 '17 at 14:55
  • 2
    \$\begingroup\$ As an improvement, 0-9 can be written as \d, with no side effects. 'use strict'; is being used twice. You can put it on the top and it takes effect everywhere. You should store document.getElementById("inputPalindrome") and document.getElementById('notification') in variables outside the click handler. Also, you have mixed quote styles. I recommend using double-quotes (") for this case. Also 'Yay! You\'ve got yourself a palindrome' is needlessly escaped, and should be in double-quotes. Also, the whole code should be wrapped inside a window.onload = function(){ ... };. \$\endgroup\$ – Ismael Miguel Mar 24 '17 at 10:37
  • 2
    \$\begingroup\$ @IsmaelMiguel Those are really good suggestions. I've made changes accordingly except \d where I think 0-9 is more clear and consistent with a-z. Thanks for all the suggestions. :) \$\endgroup\$ – Tushar Mar 24 '17 at 11:00
9
\$\begingroup\$

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);
}());
\$\endgroup\$
  • 1
    \$\begingroup\$ I like your answer +1. Except two things + on character class is faster and if you miss a comma in multiple var declaration, following variables will be global. \$\endgroup\$ – Tushar Mar 23 '17 at 15:16
  • 1
    \$\begingroup\$ @Tushar both valid comments - there's no clear cut right or wrong either way. My approach is that both of those are (in most cases) the tiniest of micro-optimisations; over the course of your entire lifetime (not just the application's lifetime, but yours) the optimisations won't even save one second of processing time. In that scenario, I tend towards what I think is slightly more readable. You may disagree and that's totally fine, so long as you understand why you're doing it :-) \$\endgroup\$ – Joe Mar 23 '17 at 15:22
  • \$\begingroup\$ Small improvements: You should use window.onload = function(){ ... }; instead of a self-calling anonymous function. Also, you should store all document.getElementById(...) outside the event handler. You did the right thing in using double quotes instead of escaping, but you aren't consistent about it. You should replace the single-quotes with double-quotes, to keep it consistent. Also, on your regular expression, you can write \d instead of 0-9. You are using 'use strict'; twice. Put it as the first line and you're done. [...] \$\endgroup\$ – Ismael Miguel Mar 24 '17 at 10:43
  • \$\begingroup\$ [...] Also, about condencing multiple vars into one may be prone to errors. It may be slightly faster, but that's it. If you forget a quote at the end, you suddenly have a global variable. Bad, bad idea... Using multiple vars is explicitly saying "This is a local variable". With a single var, you are forced to check the end of the line above to know if it is a local or a global variable. \$\endgroup\$ – Ismael Miguel Mar 24 '17 at 10:46
  • \$\begingroup\$ @IsmaelMiguel do you have a source for window.onload vs IIFE? I'm interested :-) Re consistency, I use single quotes unless I want a single quote in the value, then I'll use double quotes. In this small app it's possible to exclusively say "I only use double quotes", but in larger apps you'll want both single and double quotes in different strings. Re variable declarations; indenting tells you. Sure it's not foolproof, but with an auto-formatting and code smelling IDE it's easy to spot. Agreed on everything else \$\endgroup\$ – Joe Mar 24 '17 at 13:54
4
\$\begingroup\$

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)
\$\endgroup\$
0
\$\begingroup\$

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, ''); 
\$\endgroup\$
-2
\$\begingroup\$

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>

\$\endgroup\$
  • 1
    \$\begingroup\$ Technically true, but your isPalindrome function just went from 3 lines to 25 lines, gained 2 index variables (i and j), 3 while loops (2 of which are nested)... and that's just the obvious stuff. Regexes are a good thing, not a bad thing (so long as you know when it's appropriate to use them) \$\endgroup\$ – Joe Mar 23 '17 at 14:58
  • 2
    \$\begingroup\$ Well, apart from it actually being slower than using a regex, you may have had a point. Even if it was faster though, the complexity you add for a micro-optimisation is grossly over-doing it, unless you happen to need that optimisation. For general usage, and certainly for professional usage, you'd take the 3-line regex solution every day of the week until it's found to be causing a bottleneck \$\endgroup\$ – Joe Mar 23 '17 at 15:20
  • 1
    \$\begingroup\$ Its true that his is a lot longer than the original code but there is a lot of extra stuff. It could be shortened considerably: ...; for(let lower=0, upper=str.length-1;upper>lower; lower++, upper--) { if str[lower] != str[upper] return false }; return true. If this was an interview test this is the solution I would probably want to see. \$\endgroup\$ – Marc Rohloff Mar 23 '17 at 15:51
  • 2
    \$\begingroup\$ @MarcRohloff in that case, with the best will in the world, I hope you're never interviewing me for a job :P I disagree that it does "a lot of extra stuff" and that in an interview for a job you're looking for someone who can write 25 LOC when 3 LOC will suffice. That said, it's the Internet, you're your own person and far be it from me to criticise :-) \$\endgroup\$ – Joe Mar 23 '17 at 15:57
  • 1
    \$\begingroup\$ "Programmers waste enormous amounts of time thinking about, or worrying about, the speed of noncritical parts of their programs, and these attempts at efficiency actually have a strong negative impact when debugging and maintenance are considered. We should forget about small efficiencies, say about 97% of the time: premature optimization is the root of all evil. Yet we should not pass up our opportunities in that critical 3%." -- Donald Knuth \$\endgroup\$ – Malvolio Mar 23 '17 at 20:04

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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