The chatbot for The Community Post Pimping Board

Question

I recently wrote a chat bot for a chat room here at Code Review. The chatroom is meant to be a place where people can come and pimp a post.

That chat bot is meant to handle a list of subscribed people and a list of pimped IDs.

• People on the subscribed list can pimp posts and will get notified when someone else pimps a post in the chat room.

• Post IDs on the list are posts that have already been pimped that day. A post cannot be pimped twice in one day.

var chat = document.getElementById("chat");
var input = document.getElementById("input");
var send = document.getElementById("sayit-button");
var subscribedStorage = "SIRPYTHON_PIMPBOT_SUBSCRIBED";
var pimpedStorage = "SIRPYTHON_PIMPBOT_PIMPED";

sessionStorage.setItem(pimpedStorage, "{}"); // so I don't get an undefined thing later

/**
    Returns the last chat message spoken
*/
function getLastMessage() {
    return {
        content: chat.lastElementChild.children[1].lastElementChild.children[1].innerHTML,
        user: chat.lastElementChild.children[0].children[2].innerHTML.replace(/ /g,'')
    };
}

// ---------- Chat functions ----------
/**
    Sends a message to chat
*/
function sendMessage(message) {
    input.value = message;
    send.click();
}
/**
    Sends a message @ a user
*/
function sendTo(message, user) {
    sendMessage("@" + user + " " + message);
}
// ---------- Subscribed list functions ----------
/**
    Returns an object representing the subscribed list from storage
*/
function getSubscribedList() {
    return JSON.parse(localStorage.getItem(subscribedStorage));
}
/**
    Sets an object representing the subscribed list to storage
*/
function setSubscribedList(newList) {
    localStorage.setItem(subscribedStorage, JSON.stringify(newList));
}
/**
    Adds a user to the subscribed list
*/
function addToSubscribed(username) {
    var subscribed = getSubscribedList();
    subscribed[username] = true;
    setSubscribedList(subscribed);
}
/**
    Removes a user from the subscribed list
*/
function removeFromSubscribed(username) {
    var subscribed = getSubscribedList();
    delete subscribed[username];
    setSubscribedList(subscribed);
}
/**
    Returns if a user is subscribed
*/
function isSubscribed(username) {
    return getSubscribedList()[username] != undefined;
}
/**
    Returns all the subscribed users
*/
function getSubscribedUsers() {
    var users = [];
    var subscribed = getSubscribedList();
    for(var user in subscribed) {
        users.push(user);
    }
    return users;
}
// ---------- Pimped list functions ----------
/**
    Returns an object representing the pimped list from storage
*/
function getPimpedList() {
    return JSON.parse(sessionStorage.getItem(pimpedStorage));
}
/**
    Sets an object representing the pimped list to storage
*/
function setPimpedList(newList) {
    sessionStorage.setItem(pimpedStorage, JSON.stringify(newList));
}
/**
    Adds an ID to the pimped list
*/
function addToPimped(id) {
    var pimped = getPimpedList()
    pimped[id] = true;
    setPimpedList(pimped);
}
/**
    Returns if an ID has been pimped
*/
function wasPimped(id) {
    return getPimpedList()[id] != undefined;
}

function main() {
    var message = getLastMessage();
    var messageParts = message.content.split(" ");

    if(message.user != "SirAlfred") {
        if(message.content == "subscribe") {
            if(isSubscribed(message.user) == false) {
                addToSubscribed(message.user);
                sendTo("You have been successfully subscribed.", message.user);
            } else {
                sendTo("You are already subscribed.", message.user);
            }
        } else if(message.content == "unsubscribe") {
            if(isSubscribed(message.user) == true) {
                removeFromSubscribed(message.user);
                sendTo("You have been successfully removed.", message.user);
            } else {
                sendTo("You are not subscribed.", message.user);
            }
        } else if(messageParts[0] == "pimp") {
            if(isSubscribed(message.user) == true) {
                var id = messageParts[1];
                if(id == undefined) {
                    sendTo("You need to provide the ID of your answer.", message.user);
                }

                if(wasPimped(id) == false) {
                    addToPimped(id);
                    var groupMessage = "";
                    var subscribed = getSubscribedUsers();
                    for(var i = 0; i < subscribed.length; i++) {
                        groupMessage += ("@" + subscribed[i] + " ");
                    }
                    sendMessage(groupMessage);
                    window.setTimeout(function() {
                        sendMessage("http://codereview.stackexchange.com/a/" + id);
                    }, 4000); // to prevent the chat from blocking the message due to it being sent too early
                } else {
                    sendTo("That post has already been pimped today.", message.user);
                }
            } else {
                sendTo("You must be subscribed to pimp here.", message.user);
            }
        }
    }

    window.setTimeout(main, 5000);
}

Note: This bot is still a work in progress. This is the first working version of the bot.

There are 3 commands in the chat room:

• subscribe -- subscribes a user to the list
• unsubscribe -- unsubscribes a user from the list
• pimp postID -- pimps a post (answers will be OneBoxed, question will not).

When a post is pimped, a notification is sent out to everyone on the subscribed list. Then, the link to the post is printed out.

The subscribed list is stored as JSON in local storage. The pimped list is stored as JSON in session storage.

Concerns:

• How are my functions split up? Are there some that should be grouped together?

• Is my message checking clean? Are all those if statements readable and understandable?

Any other recommendations are welcome.

Answer 1

There are a few points that surely can be improved:

1. Since you are checking if id == undefined, I recommend you to wrap your code in an anonymous function.

Why is that? Try this: var a, undefined=5; alert(a == undefined);. It will show false.

---

2. You repeat this a lot:

if(isSubscribed(message.user) == true) {

if(wasPimped(id) == false) {

if(isSubscribed(message.user) == false) {

I don't think you really need to compare it with false or true.

---

3. You have the following for loop:

for(var i = 0; i < subscribed.length; i++)

You really should reduce the load a bit here.
Consider the following loop:

for(var i = 0, length = subscribed.length; i < length; i++)

This reduces the time because you don't have to access the length property everytime.

---

4. You created a main function, but, where is it called?

I couldn't find anywhere where it is called, only when you pass it to setTimeout.

---

5. Your main can be changed to make it more easy to understand what is going on.

Let's take the block to pimp a user:

if(isSubscribed(message.user) == true) {
    var id = messageParts[1];
    if(id == undefined) {
        sendTo("You need to provide the ID of your answer.", message.user);
    }

    if(wasPimped(id) == false) {
        addToPimped(id);
        var groupMessage = "";
        var subscribed = getSubscribedUsers();
        for(var i = 0; i < subscribed.length; i++) {
            groupMessage += ("@" + subscribed[i] + " ");
        }
        sendMessage(groupMessage);
        window.setTimeout(function() {
            sendMessage("http://codereview.stackexchange.com/a/" + id);
        }, 4000); // to prevent the chat from blocking the message due to it being sent too early
    } else {
        sendTo("That post has already been pimped today.", message.user);
    }
} else {
    sendTo("You must be subscribed to pimp here.", message.user);
}

Yeah, it's confusing. It took me 5 minutes to understand what is required to be pimped.

Consider the following code:

if( !isSubscribed(message.user) ) {
    sendTo("You must be subscribed to pimp here.", message.user);
} else {
    var id = messageParts[1];

    if( !id ) {
        sendTo("You need to provide the ID of your answer.", message.user);
    }
    else if( wasPimped(id) ) {
        sendTo("That post has already been pimped today.", message.user);
    }
    else {
        addToPimped(id);
        var groupMessage = "";
        var subscribed = getSubscribedUsers();
        for(var i = 0, length = subscribed.length; i < length; i++) {
            groupMessage += ("@" + subscribed[i] + " ");
        }
        sendMessage(groupMessage);
        window.setTimeout(function() {
            sendMessage("http://codereview.stackexchange.com/a/" + id);
        }, 4000); // to prevent the chat from blocking the message due to it being sent too early
    }
}

Each condition is in a logical order and easy to understand. You know what you need to do to pimp someone.

Answer 2

If you're going to add more commands in the future, I'd recommend setting up a dictionary/object, or a switch statement. Personally, I'd recommend the switch statement, so here's how you might do that:

if(message.user !== "SirAlfred") {
    switch(message.content) {
        case "subscribe":
            ...
            break;

        ...
    }
}

Also, you have a bug. Instead of using the != and == equality operators, you should be using the !== and === identity operators. See this Stackoverflow question.

Answer 3

A note on comments...

Almost all of your comments already say what the function code says, since you are using clear naming. However, you have a behemoth function main() that is completely void of comments. Perhaps it should be the other way around...?

Answer 4

Your chatbot makes two assumptions:

1. The only thing that anyone will want to "pimp" is an answer.
2. No one wants "share" credit for the answer they're pimping.

If I wanted to share this question, for example, ordinarily, I'd give someone this link:

http://codereview.stackexchange.com/q/95708/36366

The q indicates that this is a question rather than an answer (although, it seems to still work with an a there even though that's a question id).

More importantly, the /36366 is my Code Review user id. I get credit for sharing this post any time anyone navigates to the page through that link with my id at the end. It's how you get the Announcer/Booster/Publicist badges.

So, perhaps instead, the bot should accept a string that is in the format of:

[q/a]/postID/userID

So that when it pimps the link, it can appropriately build the link using a q or a appropriately and letting the pimping user get the share credit for the post.