2
\$\begingroup\$

I'm building a simple chatApp. The UI has just 3 interaction areas: input, submit and previous messages button.

But I'm not interested in reviewing the app architecture as much as the javascript ideas and general understanding/errors.

I've commented the code, and would like to see if you can point out any bad practice. I'm sure most of the code is bad, but still, the most important errors.

I've also included the UI as a runnable snippet.

Any edits to make it better for reviewers will be done.

<!doctype html>
<html>
  <head>
    <title>Socket.IO chat</title>
    <style>
    * { margin: 0; padding: 0; box-sizing: border-box; }
body { font: 13px Helvetica, Arial; }
form { background: #000; padding: 3px; position: fixed; bottom: 0; width: 100%; }
form input { border: 0; padding: 10px; width: 90%; margin-right: 0.5%; }
form button { width: 9%; background: rgb(130, 224, 255); border: none; padding: 10px; }
#messages { list-style-type: none; margin: 0; padding: 0; }
#messages li { padding: 5px 10px; }
#messages li:nth-child(odd) { background: #eee; }

#wantMore{
  position:absolute;
  top:0;
  right:0;
}
</style>
    </head> 
  <body>
    <button id="wantMore">More</button>
    <ul id="messages"></ul>
    <form action="">
      <input id="m" autocomplete="off" /><button>Send</button>
    </form>
    <script src="mySnippet"></script>
    <script src="socket-io.js"></script>
  </body>
</html>

const prevMsgs = document.querySelector("button#wantMore")
const uri = "http://localhost:3000"

class Msg {
  constructor (msg, createdAt){
    this.msg = msg
    this.createdAt = createdAt
  }
static toHTML ({msg, createdAt}) {
// pass object, builds list item.
  try{
  //in case the date conversion fails.
    createdAt = (new Date(createdAt))
      .toTimeString()
      .substr(0,5)
  } catch(e){ console.log(e) } 
  finally{ createdAt || "" }
return `<li><span>${msg}</span> &nbsp; <span>${createdAt}</span></li>`
}

static put (data) {
  $("#messages").prepend(this.toHTML(data))
}

static getMsgs (uri, skip, limit) {
  //returns a promise, errors are catch
  return fetch(`${uri}/messages/skip/${skip}/limit/${limit}`)
    .then(data => data.json())
    .then(items => items.forEach(item => this.put(item)))
}

static getLastMsgs (uri) { return this.getMsgs(uri, 0,10) }

}


$(function () { // $(document).ready(
  
  Msg.getLastMsgs(uri).catch(e => console.log(e))

  prevMsgs.addEventListener("click", () => {
      const msgLen = parseInt($("#messages").children().length)
      Msg.getMsgs(uri, msgLen, msgLen+10)
    })

  const socket = io(); //connects to / origin

  $('form').submit(function(e) {
    e.preventDefault(); // prevents page reloading

    const msg = new Msg($('#m').val(), new Date())

    //for the sender, append right away.
    $("#messages").append(Msg.toHTML(msg))
    // and emit, and broadcast
    socket.emit('chat message', msg);
    $('#m').val('');
    return false;
  });
  
  socket.on('chat message', data => {
    $("#messages").append(Msg.toHTML(data)) 
  })
})

```
Improve this question
\$\endgroup\$
1
  • \$\begingroup\$ The current question title, which states your concerns about the code, applies to too many questions on this site to be useful. The site standard is for the title to simply state the task accomplished by the code. Please see How do I ask a good question?. \$\endgroup\$ – BCdotWEB Dec 26 '20 at 17:20
1
\$\begingroup\$

Be careful of IDs, especially those with common names, since they (unfortunately) automatically create properties on the global object when the ID is a valid identifier, which is not an uncommon source of bugs 2. Here, you have #messages, which is probably a pretty common word related to your script. It's not likely to be a problem in most cases, but I'd feel more comfortable using a class instead.

CSS tweak Due to positioning, the send button appears to have an unusually thick border on the right. Consider using width: 9.5%; instead of width: 9%;. You could also consider using flex for the <form> instead, and only set an explicit width of a few % for the button, letting the <input> horizontally expand to take up the rest of the space automatically, without hard-coding.

jQuery? You have a few uses of $ that look like uses of jQuery, but it doesn't appear to exist as a <script> in the HTML. Make sure to include it. If jQuery is included via a build process like Webpack (which is a perfectly reasonable approach) but simply not listed in the code here, consider also including socket.io the same way, for consistency and to reduce the number of requests the client has to make before the page becomes operational.

If you are indeed intending to use jQuery, then for stylistic consistency, consider using it for DOM selection and event listeners everywhere, rather than sometimes using jQuery and sometimes using querySelector and addEventListener.

Indentation Consistent indentation improves readability, when one can understand the { } blocks at a glance. Lots of IDEs have an auto-formatting feature, consider taking advantage of it. Eg

  }
static toHTML ({msg, createdAt}) {
// pass object, builds list item.
  try{

should be

  }
  static toHTML ({msg, createdAt}) {
    // pass object, builds list item.
    try {

Creation of invalid dates won't throw - the try/catch won't do anything. Worst case, createdAt will end up containing the string 'Inval' (from Invalid Date). The finally has an unused expression too - did you mean to assign to createdAt maybe? To check if the date is invalid, you could either check if the time string is Invalid Date first, or see if getTime returns NaN.

Msg class That whole class seems odd to me, since it never uses its instance properties and only has static methods. The fact that the structure is a class isn't providing any organizational benefit for the code. Consider either:

  • Use at least some prototype methods for the class instead, when the method uses data that should exist on an instance - so, toHTML and put. (put isn't a very informative name, maybe rename to insertIntoDOM or renderMessage)
  • Remove the class entirely and use a plain array of objects and plain functions instead

Be careful when concatenating HTML with user input, because it can result in arbitrary code execution. With this:

`<li><span>${msg}</span>...

If msg contains unsafe code (either posted by another user, or if this user has been tricked into entering unsafe code), the current user could have all available sensitive data sent to a malicious third party.

To fix it, create the <span> without any content at first, then assign to its text content afterwards, maybe something like:

// createdAt looks like it should always be trustworthy, so interpolation is OK
const $li = $(`<li><span></span> &nbsp; <span>${createdAt}</span></li>`);
$li.find('span').text(msg);
return $li;

Catch errors If the fetch fails, it will not be caught if initiated from a click. Add a .catch onto Msg.getMsgs(uri, msgLen, msgLen+10). In both this new .catch and the existing .catch on the getLastMsgs call, when there's an error, it'd be user-friendly to display the fact that there was an error to the user, perhaps in a popup, rather than just logging. After all, while developers look at the console to debug, normal users don't.

Improve this answer
\$\endgroup\$
9
  • \$\begingroup\$ I've just downloaded a minified, slim version of webpack from their site, do you think that's a reasonable approach? \$\endgroup\$ – Minsky Dec 26 '20 at 16:04
  • \$\begingroup\$ also the link to global variables is C/C++ is that what you intended? \$\endgroup\$ – Minsky Dec 26 '20 at 16:08
  • \$\begingroup\$ Via NPM, I hope? If so, yeah - see webpack.js.org/guides/getting-started for examples. It's somewhat complicated. It's well worth it in larger projects, or maybe in smaller projects if you're already familiar with how it works, but if you haven't used it before I wouldn't bother for something this small. \$\endgroup\$ – CertainPerformance Dec 26 '20 at 16:08
  • 1
    \$\begingroup\$ Despite the question being tagged with C++, the top answer there applies to problems with global variables in most languages. \$\endgroup\$ – CertainPerformance Dec 26 '20 at 16:09
  • 1
    \$\begingroup\$ It is a string, but it may be an unsafe string that contains untrustworthy code. Consider if someone not very tech-savvy gets an email or something saying: "Copy and paste in the following text into the message box, you won't believe what happens next!" - and then their personal information and login credentials get stolen. Eg <img src onerror="alert('evil')"> but with the alert("evil") replaced with malicious code. \$\endgroup\$ – CertainPerformance Dec 26 '20 at 19: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.