6
\$\begingroup\$

This is a really simple chat room that I've made using Firebase. Before I take it to the next level I want to know if there is anything I can improve right now. Since I'm new to Firebase I don't know a whole lot about it so any pointers are greatly appreciated.

var myDataRef = new Firebase('https://insertnamehere.firebaseio.com/');

//Set a max limit of entries to 100
dataAddQuery = myDataRef.limit(100);

dataAddQuery.on('child_added', function(snapshot) {
    var message = snapshot.val();
    displayChatMessage(message.name, message.text);
});

//Adds a scrollbar when 26 messages have been written so that it stays within the container
dataRemoveQuery = myDataRef.endAt().limit(26);

dataRemoveQuery.on('child_removed', function(snapshot) {
    var message = snapshot.val();
    removeChatMessage(message.name, message.text);
});

document.getElementById("messageInput").addEventListener("keypress",(function (e) {

    //When ENTER is pressed
    if (e.keyCode == 13) {
        var name = document.getElementById('nameInput').value;

        if (name.length > 18) {
            alert('Please enter a shorter name'); //The alert is temporary
        }
        else {
            var text = document.getElementById('messageInput').value;

            if (text.length > 450) {
                alert('Your message is too long'); //The alert is temporary
            }
            else {
                myDataRef.push({name: name, text: text});
                document.getElementById('messageInput').value = "";
            }
        }
    }
}))

function displayChatMessage(name, text) {

  var newMessage = document.createElement("div");
  newMessage.innerHTML = "<b>" + name + "</b> : " + text;
  document.getElementById("messagesDiv").appendChild(newMessage);
}

function removeChatMessage(name, text) {
  document.getElementById("messagesDiv").removeChild(document.getElementById("messagesDiv").firstChild)

}

HTML: 

<body>

<div id='container'>
    <div id='header'>Have a chat</div> 
    <div id='messagesDiv'></div>

    <div id='inputs-holder'>
        <label for='nameInput'>Name:</label>
        <input type='text' id='nameInput'>
        <!--<span>Press enter to send your message</span> Excluded for this review -->
        <!--<button id='questionmark'>?</button> Excluded for this review -->
        <textarea type='text' id='messageInput' placeholder='Write your message here...'></textarea>   
    </div>
</div>

<!--<script src='https://ajax.googleapis.com/ajax/libs/jquery/1.11.1/jquery.min.js'></script>-->
<script src="chatroom.js"></script>
</body>
Improve this question
\$\endgroup\$

2 Answers 2

Reset to default
1
\$\begingroup\$

It's strange that you've created two different Firebase references here to monitor the same data (one monitoring 100 elements and one monitoring 20). You would probably want to re-use the same ref so that they are consistent. Otherwise, when items move out of the window for your child_added list, they will not trigger child_removed calls.

var myDataRef = new Firebase('https://insertnamehere.firebaseio.com/');

// Set a max limit of entries to 100
dataQuery = myDataRef.endAt().limit(100);

// Listen for items moving into our out of the display window
dataQuery.on('child_added', /* display */);
dataQuery.on('child_removed', /* remove */);

It also looks like your remove method wouldn't work as expected. It tries to look messagesDiv inside of messagesDiv, which seems incorrect.

An easier approach would be to give each chat message a unique id. With a library like jQuery, I'd use something like data-id="<name>", but in vanilla JavaScript, I'd probably just go for the id attribute to keep things simple.

function displayChatMessage(name, text) {
  var newMessage = document.createElement("div");
  newMessage.innerHTML = "<b>" + name + "</b> : " + text;
  // add the id!
  newMessage.id = "message-" + name;
  document.getElementById("messagesDiv").appendChild(newMessage);
}

function removeChatMessage(name, text) {
  // remove using the id attribute
 var child = document.getElementById("message-"+name);
 document.getElementById("messagesDiv").removeChild(child);
}
Improve this answer
\$\endgroup\$
1
\$\begingroup\$

A short review of the 'keypress' handler;

  • I would make a dedicated named function for this handler, because of the size of it
  • Instead of only continuing when keyCode is 13, I would return if the keyCode is not 13
  • I would have a pattern where I call a function that takes data and updates an output array for messages

So something like this:

document.getElementById("messageInput").addEventListener("keypress", keypressHandler );

function keypressHandler(e) {
    //Leave unless ENTER is pressed
    if (e.keyCode != 13) {
        return;
    }
    //What should we do
    var steps = [ validateUserName, validateMessage, sendMessage ],
        output = [];

    for( var i = 0 ; i < steps.length ; i++ ){
        var f = steps[i];
        if( !f( myDataRef ) ){
          break;
      }
    }
}

And then create for each functionality it's own function that receives an output array, and returns false if processing should stop.

Improve this answer
\$\endgroup\$
1
  • \$\begingroup\$ Question, since messageInput is a textarea, why would I need to return if the key pressed is not enter? That just means I keep writing the message and nothing happens. What's the advantage of your solution there? \$\endgroup\$
    – Chrillewoodz
    Nov 5, 2014 at 7:07

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

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