4
\$\begingroup\$

I'm a web development student, and I made a chat bot that emulates an SMS app commonly found on smartphones, etc. It responds with random lorem ipsum text. I'm wondering if my functions are too long and how/if I should refactor further. For instance, when building the chat bubbles, would using a DocumentFragment or cloneNode be better for my DOM manipulation?

It reads in a JSON object, which consists of an array of word lengths and words sorted by length (i.e. words[length][wordarray].)

Working example here.

/*
 * Main module for the lorem ipsum chat, declares local vars, calls init() on
 *  load
 *
 */

(function main() {
    "use strict";

    // Local vars
    var words,       // object for lorem ipsum JSON
        xhr,         // XMLHttpRequest object
        chatInput,   // chat input
        chatHistory; // chat history window

    /*
     * init - initializes XMLHttpRequest, reads in the words object, and adds
     * event listeners
     *
     */
    function init () {

        // Get JSON using AJAX, parse to obj
        xhr = getXHR();
        xhr.open("get", "data/words.json", true);

        xhr.send(null);
        xhr.onreadystatechange = function() {
            if (xhr.readyState === 4 && xhr.status === 200)
                words = JSON.parse(xhr.responseText);
            else
                console.log("Ready state:" + xhr.readyState + " Status: " + xhr.status);
        };

        // initialize variables
        chatInput = document.getElementById("chat");
        chatInput.addEventListener("keyup", parseText, false);
        chatHistory = document.getElementById("chat_history");
    }

    /**
     * parseText is the callback for the keyup eventlistener, and listens for
     * enter key to be pressed, signaling that the user has entered a message.
     *
     * @param {Event} event          - keyup from chatInput
     *
     */
    function parseText(event) {
        var message;

        if (event.keyCode === 13 && chatInput.value) {
            message = chatInput.value.trim();

            // message is "sent" and triggers bot "response" with small delay
            if (message !== "") {
                chatInput.value = "";
                sendMessage("user", message);
                setTimeout(function () {
                    respondTo(message);
                }, Math.random() * (4000) + 1000);
            }
        }
    }


    /**
     * sendMessage sends a message with an optional delay and posts it to the
     * .chat_history window.
     *
     * @param  {String} from       - "user", "bot" class
     * @param  {String} message    - message
     * @param  {Number} delay      - delay in MS
     *
     */
    function sendMessage(from, message, delay) {
        var p,                 // paragraph element for message
            img,               // image for avatar
            innerDiv,          // inner div to hold animation and avatar
            outerDiv,          // outer div for clearing floats
            animationSequence, // class list for animation
            position;          // left or right

        // paragraph
        p = document.createElement("p");

        // img
        img = document.createElement("img");
        if (from === "bot") {
            img.src = "img/helmet1.svg";
            position = "left";
        }
        else if (from === "user") {
            img.src = "img/user168.svg";
            position = "right";
        }
        img.classList.add("avatar", "middle", position);

        // inner div
        innerDiv = document.createElement("div");
        innerDiv.appendChild(img);
        innerDiv.classList.add(from);

        // once the delay is done, remove animation, add message
        if (delay) {
            addAnimation(innerDiv);
            setTimeout(function () {
                removeAnimation(innerDiv);
                p.appendChild(document.createTextNode(message));
                innerDiv.appendChild(p);
                chatHistory.scrollTop = chatHistory.scrollHeight;
            }, delay);
        }
        else {
            // no delay, just post it
            p.appendChild(document.createTextNode(message));
            innerDiv.appendChild(p);
        }

        //outer div
        outerDiv = document.createElement("div");
        outerDiv.appendChild(innerDiv);
        outerDiv.classList.add("full");

        // chatHistory
        chatHistory.appendChild(outerDiv);
        chatHistory.scrollTop = chatHistory.scrollHeight;
    }


    /*
     * respondTo responds to the user's message by picking random lorem ipsum
     * words from the words object.
     *
     * @param  {String} message    - incoming message string
     *
     */
    function respondTo(message) {

        var response = "", // String to hold generated response
            numWords,      // number of words in response
            numChars,      // number of characters in word
            selectedWord,  // index of selected word (by length)
            delay,         // chat bot delay in ms
            msgLength,     // number of words in @message String
            comma;         // optional comma

        // short sentences typically get short responses.
        if (message.indexOf(" ") === -1)
            msgLength = 1;
        else
            msgLength = message.split(" ").length;

        // longer sentences should get a comma
        if (msgLength > 8)
            comma = Math.ceil(msgLength / 2);

        // maximum response length is 2 more words than the incoming message
        numWords = Math.ceil(Math.random() * (msgLength + 2));

        // simulated delayed response
        delay = Math.ceil(Math.random() * (numWords + 1) * 1000) + 2500;

        // build the response
        while (numWords > 0) {

            // pick a word, but don't repeat the last one!
            do {
                numChars = wordLengthByFrequency();
                selectedWord = Math.floor(Math.random() * words[numChars].length);
            }
            while (words[numChars][selectedWord] == response.split(" ").pop());

            // Capitalize first word only
            if (!response) {
                response = capitalizeWord(words[numChars][selectedWord]);
            }
            else
                response += words[numChars][selectedWord];

            // comma?
            if (comma && numWords == comma)
                response += ',';

            numWords--;

            // last word? add punctuation, if not add a space
            response += (numWords === 0) ? getPunctuation() : " ";
        }

        sendMessage("bot", response, delay);
    }

    /**
     * addAnimation adds the "typing" animation to element by appending the
     * animation sequence divs to the target element.
     *
     * @param {HTMLElement} element  - the target Element
     *
     */
    function addAnimation (element) {
        var animationSequence = ["one","two","three"];

        animationSequence.forEach(function (animationClass) {
            var newDiv = document.createElement("div");
            newDiv.classList.add("bouncer", animationClass);
            element.appendChild(newDiv);
        });
    }

    /**
     * removeAnimation removes the "typing" animation by removing all of the
     * child divs of the target element.
     *
     * @param  {HTMLElement} element - the target Element
     *
     */
    function removeAnimation (element) {
        var i = element.childNodes.length - 1;

        for ( ; i >= 0; i--)
            if (element.childNodes[i].tagName === "DIV")
                element.removeChild(element.childNodes[i]);
    }


    /**
     * capitalizeWord takes in a lowercase string and returns it with the first
     * letter capitalized.
     *
     * @param  {String} word - the word to capitalize
     * @return {String}      - the capitalized word
     */
    function capitalizeWord(word) {
        return  word.charAt(0).toUpperCase() + word.slice(1);
    }


    /**
     * wordLengthByFrequency provides a Normal (Gaussian) distribution for word
     * lengths. Higher length words are called less frequently.
     *
     */
    function wordLengthByFrequency() {

        var rndm = Math.floor(Math.random() * 100);

            if (rndm <= 5)
                return 1;
            else if (rndm <= 12)
                return 2;
            else if (rndm <= 21)
                return 3;
            else if (rndm <= 34)
                return 4;
            else if (rndm <= 54)
                return 5;
            else if (rndm <= 67)
                return 6;
            else if (rndm <= 76)
                return 7;
            else if (rndm <= 81)
                return 8;
            else if (rndm <= 85)
                return 9;
            else if (rndm <= 89)
                return 10;
            else if (rndm <= 92)
                return 11;
            else if (rndm <= 94)
                return 12;
            else if (rndm <= 96)
                return 13;
            else if (rndm <= 98)
                return 14;
            else if (rndm <= 99)
                return 15;
            else if (rndm <= 100)
                return 16;
    }

    /**
     * getPunctuation returns a random punctuation mark based on frequency.
     *  There is a 10% chance of an exclamation point or question mark, and an
     *  80% chance for a period.
     *
     */

    function getPunctuation() {
        var mark = Math.ceil(Math.random() * 10);

        if (mark == 9)
            return '?';
        else if (mark == 10)
            return '!';
        else
            return '.';
    }

    /**
     * getXHR opens a new XMLHttpRequest object and returns it.
     *
     */
    function getXHR() {
        if (window.XMLHttpRequest) { // Mozilla, Safari, IE7+ ...
            xhr = new XMLHttpRequest();
        }
        else if (window.ActiveXObject) { // IE 6 and older
            xhr = new ActiveXObject("Microsoft.XMLHTTP");
        }
        else {
            window.alert("Your browser does not support AJAX.");
            return false;
        }
        return xhr;
    }

    // add event listener for page load
    window.addEventListener("load", init, false);

})();
\$\endgroup\$
3
\$\begingroup\$

The design of your getXHR function is very confusing.

First off, by the top of the code, we learn that there is a variable xhr that is in a scope for all these functions to see.

Then, looking at this xhr function, if either of the first two conditionals pass, that outer xhr variable is going to be set, and then it will be returned.

Then, looking back at the init function, you are again storing the return of getXHR in the outer xhr variable.

Why are you doing it like this? This is very confusing. As a solution, I recommend that you get rid of the outer xhr variable because you only use it in init. Then, just handle the return from getXHR.

Since the outer xhr will no longer be used, here is what getXHR will look like:

function getXHR() {
    if (window.XMLHttpRequest) { // Mozilla, Safari, IE7+ ...
        return new XMLHttpRequest();
    }
    else if (window.ActiveXObject) { // IE 6 and older
        return new ActiveXObject("Microsoft.XMLHTTP");
    }
    else {
        window.alert("Your browser does not support AJAX.");
        return false;
    }
}

You don't do any handling of the return getXHR:

xhr = getXHR();
xhr.open("get", "data/words.json", true);

Looking at your getXHR function, it could possibly return false if it can't find a suitable XHR object to work with:

window.alert("Your browser does not support AJAX.");
return false;

Well, what if this function returns false? Your code is going to try to call all these functions that a normal XHR would have, but it would fail because that value is not an XHR.

You should add a conditional checking the return of this function. If the return is false, it should just exit the function immediately; this will stop everything else.


That giant else if chain in wordLengthByFrequency is very ugly. I recommend that you create an array.

In the array, the elements would be the random values to check. Then, if a check passes against an element, you can simply return the index of that element + 1.

That would look like this:

var rndm = Math.floor(Math.random() * 100);

var name = [5, 12, 21, 34, ...];

for(var i = 0, length = name.length; i < length; i++) {
    if(rndm <= name[i]) {
        return i + 1;
    }
}

DO NOT name the array "name"; I could not think of a good name for it.


| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ You are correct. The XHR function was copied from a previous program and because it "worked," I didn't notice. I should probably encapsulate my entire XHR/JSON loading into a single function (similar to this, but lighter) and call it from init. I'll add some error handling, too. This should solve all of the problems you cited. Re: the if/else block I chose this for speed and readability, but I could certainly refactor it into a loop. \$\endgroup\$ – Scott Aug 20 '15 at 18:35
1
\$\begingroup\$
  1. You call window.addEventListener, which is only available in versions of Internet Explorer that also support the XMLHttpRequest object natively, making your getXHR function completely unnecessary. There is no need for this function at all.

  2. You have HTML tag Ids hard coded. This reduces your ability to reuse this code, and it makes it more difficult for others to drop your code into existing sites. Instead of an auto executing anonymous function, just make it a global function:

    function AutoChatBot(inputId, chatHistoryId) {
        // ...
    
        function init() {
            // ...
    
            chatInput = document.getElementById(inputId);
            chatInput.addEventListener("keyup", parseText, false);
            chatHistory = document.getElementById(chatHistoryId);
        }
    
        // ...
    }
    

    Then you can call it multiple times to create multiple chat windows. This becomes easier to drop into any page built by any person because the HTML tag Ids are parameterized. This means they can retrofit this to their existing site with minimal changes.

    AutoChatBot("chat1", "chatHistory1");
    AutoChatBot("chat2", "chatHistory2");
    
| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ Thanks, I've refactored my XHR to only check for the XMLHttpRequest and alert an error otherwise. This was in part in response to the comment from @SirPython above. In regards to #2, the app isn't designed to be instantiated in such a way that you'd have multiple chat windows open at the same time, nor do I see how that would make much sense given that I am trying to emulate an SMS-type application. Also, I'd like my function to stay anonymous and outside of the global namespace as a personal preference.Thank you for the feedback! \$\endgroup\$ – Scott Aug 22 '15 at 16:57

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.