2
\$\begingroup\$

This fiddle has an ADD button when you click it a Chat window is added. When there are more than 1 windows the message from one window is reflected across all the windows.

Say you have templates (in above case "Chat Window") which will repeat in DOM (i.e. append and removal will happen dynamically):

var html="<h1>Chat Window</h1> <div> <p></p> 
<textarea rows='4' cols='50'>  </textarea>     
<input type='button' value='send'></div>";

The above template keeps repeating in the DOM i.e. when you click on the ADD button they are added inside:

<div id="chatApp">

</div>

I have an event handler which listens to the clicks in its innerHTML:

  var e = document.getElementById('chatApp');
e.addEventListener("click", function(event){
 var chatWindows=document.getElementsByTagName('p');
if(event.target.value=="send"){
    for(var i=0;i<chatWindows.length;++i){
        chatWindows[i].innerHTML+= "<em>Chat window</em> {index} says: <b>"+ event.target.previousElementSibling.value+"</b><br/>";
    }
    event.target.previousElementSibling.value=null;
}
},false);

I have a couple of questions regarding it:

  1. Say each of the template has an event with it and a respective handler so say I add this template 10 times in DOM (i.e. click add button in the fiddle 10 times) then I want to do event handling for each of them, possible ways:

    1. I append event handler to each of this template by giving the elements ID so I will have 10 event handlers which I feel is inefficient.
    2. I have one event handler on the parent of these chat windows and then use event.target to extract the particular values (like I have done in fiddle as shown above). But its side effect is that it also listens to click events other then on the SEND button.

    Which strategy to use from above?

    I want to know that SEND button of which chat window was pressed i.e. chat window of what index but Ii don't want to give ID to my templates then how do I do it? this will help me to display that message is coming from which chat window:

    Chat window {index} says: " check the above fiddle

  2. You want to recognize individual/particular group of elements in JavaScript DOM the known ways are:

    1. Provide ids to the elements dynamically before appending in DOM
    2. Provide class names
    3. Provide name attribute or any of your own attributes with some values

    Among above which is most recommended? Or if there is any other way too, then please specify.

In the Fiddle provided above, when you add more than one Chat window and then you type a msg and press SEND, I select all elements with P tag and replace their innerHTML to show message across all windows. As shown above in the handler and Fiddle.

But I know it is unsafe as, if there are other P tags in the page then they too will be screwed. Can you suggest the best strategy for this? How can I make sure that only P tags meant to display the chat message have their content changed?

\$\endgroup\$
1
  • 2
    \$\begingroup\$ Welcome to Code Review! After having received answers, please do not add revised versions of your code to your question or modify your previous code in such a way that it invalidates answers. See our meta question Can I edit my own question to include revised code? for what you can do after having received answers to your question. \$\endgroup\$ Jul 24, 2014 at 12:10

1 Answer 1

4
\$\begingroup\$

Formatting Improvements

I don't have time for a full review, but I will help with some of these formatting issues.

Quoted sources: Douglas Crockford


1... You've got some invalid code in there. "Quotes" in JavaScript can't span multiple lines.

Invalid:

var html="<h1>Chat Window</h1> <div> <p></p>
<textarea rows='4' cols='50'>  </textarea>     
<input type='button' value='send'></div>";

Valid:

var html = "<h1>Chat Window</h1> <div> <p></p>" +
        "<textarea rows='4' cols='50'>  </textarea>" + 
        "<input type='button' value='send'></div>";

2... Assignment operators, =, should be padded by a space.

Messy:

html="

Neat:

html = "

3... Variable names should be human readable.

Messy:

var e = document.getElementById('chatApp');

Neat:

var chatAppElem = document.getElementById('chatApp');

4... The parentheses of anonymous function literals should have one space of padding on either side.

Messy:

e.addEventListener("click", function(event){

Neat:

e.addEventListener("click", function (event) {

If a function literal is anonymous, there should be one space between the word function and the ( (left parenthesis). If the space is omited, then it can appear that the function's name is function, which is an incorrect reading. -DC


5... Spaces should only be made in intervals of 4.

Messy:

  var e = document.getElementById('chatApp');
e.addEventListener("click", function(event){
 var chatWindows=document.getElementsByTagName('p');

Neat:

var e = document.getElementById('chatApp');
e.addEventListener("click", function(event){
    var chatWindows=document.getElementsByTagName('p');

The unit of indentation is four spaces. Use of tabs should be avoided because (as of this writing in the 21st Century) there still is not a standard for the placement of tabstops. The use of spaces can produce a larger filesize, but the size is not significant over local networks, and the difference is eliminated by minification. -DC


6... Lines should be no longer than 80 characters.

Messy:

chatWindows[i].innerHTML+= "<em>Chat window</em> {index} says: <b>"+ event.target.previousElementSibling.value+"</b><br/>";

Neat:

chatWindows[i].innerHTML+= "<em>Chat window</em> {index} says: <b>" +
    event.target.previousElementSibling.value+"</b><br/>";

Avoid lines longer than 80 characters. When a statement will not fit on a single line, it may be necessary to break it. Place the break after an operator, ideally after a comma. A break after an operator decreases the likelihood that a copy-paste error will be masked by semicolon insertion. The next line should be indented 8 spaces. -DC


7... Comments should be used generously. And remember: "Why" not "How".

Bad Practice:

  var e = document.getElementById('chatApp');
e.addEventListener("click", function(event){
 var chatWindows=document.getElementsByTagName('p');
if(event.target.value=="send"){
    for(var i=0;i<chatWindows.length;++i){
        chatWindows[i].innerHTML+= "<em>Chat window</em> {index} says: <b>"+ event.target.previousElementSibling.value+"</b><br/>";
    }
    event.target.previousElementSibling.value=null;
}
},false);

Good Practice:

// Trigger a chat window when button is clicked.

var chatAppElem = document.getElementById('chatApp');      // Info about the purpose 
chatAppElem.addEventListener("click", function (event) {   // of significant lines
var chatWindows = document.getElementsByTagName('p');      // will be a plus later on.
if(event.target.value == "send"){
    for(var i = 0; i < chatWindows.length; ++i){
        chatWindows[i].innerHTML+= "<em>Chat window</em> {index} says: <b>" +
            event.target.previousElementSibling.value + "</b><br/>";
    }
    event.target.previousElementSibling.value = null;
}
},false);

Be generous with comments. It is useful to leave information that will be read at a later time by people (possibly yourself) who will need to understand what you have done. The comments should be well-written and clear, just like the code they are annotating. An occasional nugget of humor might be appreciated. Frustrations and resentments will not.

It is important that comments be kept up-to-date. Erroneous comments can make programs even harder to read and understand.

Make comments meaningful. Focus on what is not immediately visible. Don't waste the reader's time with stuff like

i = 0; // Set i to zero.

Generally use line comments. Save block comments for formal documentation -DC

\$\endgroup\$
1
  • \$\begingroup\$ thanks for your response. Sorry for a hasty post in the first place but now i have incorporated most of the changes suggested by you. I will take care that i always follow it. \$\endgroup\$ Jul 24, 2014 at 9:09

Your Answer

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

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