37
\$\begingroup\$

This is the first JavaScript code I've ever written, alert("hello, world!"); aside.


This site is closing quite a lot of questions because people won't read the rulebook or pay attention to the How To Ask panel that's displayed as you're entering your question title - almost 60% of closed questions involve broken code, so I suggested to pop a red in-your-face warning when a user is about to ask a blatantly off-topic question with broken code.

Not knowing whether Stack Exchange would go forward with something like that, I suggested we implement one ourselves, for the heck of it. It's still too early for the July , so this isn't an entry... well it would be, but I just couldn't wait to try something, so here it goes.


I've "stolen" some HTML and CSS from the Ask Question page, adapted it a little bit to fit the Stack Snippet box, and proceeded to implement the JavaScript code to make it work.

Go ahead, try it! The script is triggered when you hit ENTER in the title box.

NOTE: The code uses String.includes, which may not be available in your browser. See the accepted answer for details.

function validateKey(e) {
  "use strict";
  if (e.keyCode == 13) {
    var field = document.getElementById("title");
    if (!validateTitle(field)) {
      showWarning();
    }
    else {
      clearWarning();
    }
  } 
}


function validateTitle(titleField) {
  "use strict";
  var title = titleField.value;
  return !title.includes("bug") 
      && !title.includes("issue") 
      && !title.includes("t work") // catches won't work, isn't working, etc.
      && !title.includes("s wrong") // catches what's wrong, what is wrong
      && !title.includes("fix")
      && !title.includes("why");
}


function showWarning() {
  "use strict";
  var msg = getWarningDiv();
  document.getElementsByTagName("table")[0].appendChild(msg);
}

function clearWarning() {
  "use strict";
  var popup = document.getElementsByClassName("message-dismissable")[0];
  if (popup == undefined) {
    return;
  }
  
  popup.parentNode.removeChild(popup);
}

function getWarningDiv() {
  "use strict";
  
  var popup = document.createElement("div");  
  popup.className = "message message-error message-dismissable";
  popup.setAttribute("style", "max-width: 270px; min-width: 270px; position: absolute; top: 32px; left: 378px; display: block;");
  popup.addEventListener("click", function(event) { clearWarning(); });
  
  var tooltip = document.createElement("div");
  tooltip.className = "message-inner message-tip message-tip-left-top";
  
  popup.appendChild(tooltip);
  
  var tipText = document.createElement("div");
  tipText.title = "close this message (or hit Esc)";
  tipText.className = "message-close";
  tipText.textContent  = "×";
  
  tooltip.appendChild(tipText);
  
  var msg = document.createElement("div");
  msg.className = "message-text";
  msg.setAttribute("style", "padding-right: 35px;");
  msg.textContent = "Wait! If your code does not work as intended, this question is off-topic!";
  
  popup.appendChild(msg);
  return popup;
}
body {
  font-family: 'Helvetica Neue',Helvetica,Arial,sans-serif;
  font-size: 13px;
  line-height: 1.3em;
  color: #222;
  background: #fff;
  min-width: 1030px;
}

.ask-title-table {
  width: 668px;
  height: 44px;
}

table {
  border-collapse: collapse;
  border-spacing: 0;
  border-color: grey;
}

.ask-title-cell-key {
  width: 40px;
}

#title {
  width: 498px;
}

.form-item label {
  display: block;
  font-weight: bold;
  padding-bottom: 3px;
}

input[type=text], input[type=url], input[type=email], input[type=tel], textarea {
  font-family: 'Helvetica Neue',Helvetica,Arial,sans-serif;
  background: #fff;
  color: #444;
  border: 1px solid #ccc;
  font-size: 14px;
  padding: 8px 10px;
}

.message.message-error.message-dismissable {
  cursor: pointer;
}

.message.message-error {
  z-index: 1;
  display: none;
  color: #fff;
  background-color: #c04848;
  text-align: left;
}

.message.message-error .message-text {
  padding: 15px;
}

.message.message-error .message-close {
  padding: 2px 6px 3px 6px;
  font-family: Arial,sans-serif;
  font-size: 16px;
  font-weight: normal;
  color: #fcb2b1 !important;
  line-height: 1;
  float: right;
  border: 1px solid rgba(255,255,255,0.2);
  margin-top: 8px;
  margin-right: 8px;
}

.message.message-error .message-tip-left-top:before {
  top: 0;
  left: -9px;
  border-top: 9px solid #c04848;
  border-left: 9px solid transparent;
}

.message.message-error .message-tip:before {
  content: "";
  position: absolute;
}
<script src="https://ajax.googleapis.com/ajax/libs/jquery/2.1.1/jquery.min.js"></script>
<html>
  <head>
      <title>Ask a Question - Code Review Stack Exchange</title>
  </head>
  <body>
    <form>
    <div id="question-form">
      <div class="form-item ask-title">
        <table class="ask-title-table">
          <tbody>
            <tr>
              <td class="ask-title-cell-key">
                <label for="title">Title</label>
              </td>
              <td class="ask-title-cell-value">
                <input id="title" name="title" type="text" maxlength="300" tabindex="100" placeholder="State the task that your code accomplishes. Make your title distinctive." class="ask-title-field" data-min-length="15" data-max-length="150" autocomplete="off" onkeypress="return validateKey(event)"/>
              </td>
            </tr>
          </tbody>
        </table>
      </div>
      <!-- warning should be inserted here -->
    </div>
    </form>
  </body>
</html>


Again, the CSS and HTML aren't exactly mine - they're merely an excuse for the JavaScript, which I'd like to improve. Also, did I overlook any blatant false positives?

\$\endgroup\$
10
  • \$\begingroup\$ Is the JavaScript all yours ? Just asking so I don't pick on stuff you didn't write. \$\endgroup\$
    – janos
    Commented Jun 12, 2015 at 6:46
  • \$\begingroup\$ @Janos the JavaScript is all mine :) \$\endgroup\$ Commented Jun 12, 2015 at 6:47
  • 1
    \$\begingroup\$ Is this snippet supposed to do anything? \$\endgroup\$
    – njzk2
    Commented Jun 12, 2015 at 16:19
  • 2
    \$\begingroup\$ @njzk2 see 200_success' answer - turns out the string.includes() function only works in Chrome. \$\endgroup\$ Commented Jun 12, 2015 at 16:21
  • 1
    \$\begingroup\$ @Lyle'sMug eh.. it's not Lyle-proof! ;-) \$\endgroup\$ Commented Jun 13, 2015 at 1:12

5 Answers 5

31
\$\begingroup\$

String.includes() is part of the experimental ECMAScript 6 language proposal. Therefore, you cannot rely on it being available in browsers. On browsers that do not support String.includes(), your script does nothing — it just leaves an error in the JavaScript console, if one cares to look there. That is one of the dangers of JavaScript programming: the page just breaks completely due to one mistake, even if it works on your own browser.

The conservative approach, title.indexOf(…) >= 0, is definitely recommended.

Other than that, your code looks pretty good. However, since you have included jQuery in your Stack Snippet, I am puzzled by why you didn't make use of it. Much of this code would be simplified.

More jQuery, please!

\$\endgroup\$
2
  • 5
    \$\begingroup\$ jQuery fixes everything... \$\endgroup\$
    – RubberDuck
    Commented Jun 12, 2015 at 12:49
  • 1
    \$\begingroup\$ It's the batman to regex's joker. \$\endgroup\$
    – Vincent
    Commented Jun 12, 2015 at 13:47
26
\$\begingroup\$

The validateTitle method takes a titleField, and then extracts titleField.value as title. Since the main responsibility of this function is to validate a string, it would be better to do just that, without the added knowledge that it has to extract the string in question by calling .value on the field it received. In short, just pass it the field value directly.


This is of course about the enter key:

  if (e.keyCode == 13) {

But this way it's more obvious and easier to read:

  var ENTER_KEY = 13;
  // ...

  if (e.keyCode == ENTER_KEY) {

Instead of the repetitive !title.includes with the string patterns buried in the code:

  // ...
  && !title.includes("issue") 
  && !title.includes("t work") // catches won't work, isn't working, etc.
  // ...

I suggest to put the suspicious strings in an array near the top of the code, and replace the repetitive !title.includes with a loop:

var INVALID_STRINGS = [
    'bug',
    'issue',
    't work',   // catches won't work, isn't working, 
    's wrong',  // catches what's wrong, what is wrong
    'fix',
    'why'
];

// ...

  for (var i = 0; i < INVALID_STRINGS.length; ++i) {
      if (title.includes(INVALID_STRINGS[i])) {
          return false;
      }
   }
   return true;

I like to make the distinction between two kinds of validation methods:

  • validateSomething:
    • Check if "something" is valid, and if not, throw an exception.
    • Alternatively, check if "something" is valid, and do different things depending on success / failure
  • isValidSomething:
    • Check if "something" is valid and return true or false accordingly

In this spirit, I would rename validateTitle to isValidTitle


See the runnable snippet with the above changes applied.

var ENTER_KEY = 13;
var INVALID_STRINGS = [
    'bug',
    'issue',
    't work',   // catches won't work, isn't working, 
    's wrong',  // catches what's wrong, what is wrong
    'fix',
    'why'
];

function validateKey(e) {
  "use strict";
  if (e.keyCode == ENTER_KEY) {
    var field = document.getElementById("title");
    if (!isValidTitle(field.value)) {
      showWarning();
    } else {
      clearWarning();
    }
  } 
}

function isValidTitle(title) {
  "use strict";
  for (var i = 0; i < INVALID_STRINGS.length; ++i) {
      if (title.includes(INVALID_STRINGS[i])) {
          return false;
      }
   }
   return true;
}

function showWarning() {
  "use strict";
  var msg = getWarningDiv();
  document.getElementsByTagName("table")[0].appendChild(msg);
}

function clearWarning() {
  "use strict";
  var popup = document.getElementsByClassName("message-dismissable")[0];
  if (popup == undefined) {
    return;
  }
  
  popup.parentNode.removeChild(popup);
}

function getWarningDiv() {
  "use strict";
  
  var popup = document.createElement("div");  
  popup.className = "message message-error message-dismissable";
  popup.setAttribute("style", "max-width: 270px; min-width: 270px; position: absolute; top: 32px; left: 378px; display: block;");
  popup.addEventListener("click", function(event) { clearWarning(); });
  
  var tooltip = document.createElement("div");
  tooltip.className = "message-inner message-tip message-tip-left-top";
  
  popup.appendChild(tooltip);
  
  var tipText = document.createElement("div");
  tipText.title = "close this message (or hit Esc)";
  tipText.className = "message-close";
  tipText.textContent  = "×";
  
  tooltip.appendChild(tipText);
  
  var msg = document.createElement("div");
  msg.className = "message-text";
  msg.setAttribute("style", "padding-right: 35px;");
  msg.textContent = "Wait! If your code does not work as intended, this question is off-topic!";
  
  popup.appendChild(msg);
  return popup;
}
body {
  font-family: 'Helvetica Neue',Helvetica,Arial,sans-serif;
  font-size: 13px;
  line-height: 1.3em;
  color: #222;
  background: #fff;
  min-width: 1030px;
}

.ask-title-table {
  width: 668px;
  height: 44px;
}

table {
  border-collapse: collapse;
  border-spacing: 0;
  border-color: grey;
}

.ask-title-cell-key {
  width: 40px;
}

#title {
  width: 498px;
}

.form-item label {
  display: block;
  font-weight: bold;
  padding-bottom: 3px;
}

input[type=text], input[type=url], input[type=email], input[type=tel], textarea {
  font-family: 'Helvetica Neue',Helvetica,Arial,sans-serif;
  background: #fff;
  color: #444;
  border: 1px solid #ccc;
  font-size: 14px;
  padding: 8px 10px;
}

.message.message-error.message-dismissable {
  cursor: pointer;
}

.message.message-error {
  z-index: 1;
  display: none;
  color: #fff;
  background-color: #c04848;
  text-align: left;
}

.message.message-error .message-text {
  padding: 15px;
}

.message.message-error .message-close {
  padding: 2px 6px 3px 6px;
  font-family: Arial,sans-serif;
  font-size: 16px;
  font-weight: normal;
  color: #fcb2b1 !important;
  line-height: 1;
  float: right;
  border: 1px solid rgba(255,255,255,0.2);
  margin-top: 8px;
  margin-right: 8px;
}

.message.message-error .message-tip-left-top:before {
  top: 0;
  left: -9px;
  border-top: 9px solid #c04848;
  border-left: 9px solid transparent;
}

.message.message-error .message-tip:before {
  content: "";
  position: absolute;
}
<script src="https://ajax.googleapis.com/ajax/libs/jquery/2.1.1/jquery.min.js"></script>
<html>
  <head>
      <title>Ask a Question - Code Review Stack Exchange</title>
  </head>
  <body>
    <form>
    <div id="question-form">
      <div class="form-item ask-title">
        <table class="ask-title-table">
          <tbody>
            <tr>
              <td class="ask-title-cell-key">
                <label for="title">Title</label>
              </td>
              <td class="ask-title-cell-value">
                <input id="title" name="title" type="text" maxlength="300" tabindex="100" placeholder="state the task that your code accomplishes. Make your title distinctive." class="ask-title-field" data-min-length="15" data-max-length="150" autocomplete="off" onkeypress="return validateKey(event)"/>
              </td>
            </tr>
          </tbody>
        </table>
      </div>
      <!-- warning should be inserted here -->
    </div>
    </form>
  </body>
</html>

\$\endgroup\$
16
\$\begingroup\$

I'm surprised to see that nobody else has mentioned this but you forgot one thing:

THERE IS A BUG IN MY CODE

is an accepted title, while

there is a bug in my code

is not.

You really should make it case-insensitive.


The String-contains approach is an easy approach to take on this issue, but it can also lead to false positives:

  • "Tracking issues on Github with a Python script"
  • "Stop bugging me! (A game about bugs)"

In this case though, it doesn't hurt that much IMO to have false-positives, and I must say that your simple filter is quite good.

\$\endgroup\$
1
  • 11
    \$\begingroup\$ Oh crap, there's a bug in my code! \$\endgroup\$ Commented Jun 12, 2015 at 18:01
13
\$\begingroup\$

A couple of small things:

"use strict";

Seeing as you want all of your functions to use strict mode, just put it at the top of the file instead. (Note that only comments and white space are allowed before it.)

    if (!validateTitle(field)) {
      showWarning();
    }
    else {
      clearWarning();
    }
  if (popup == undefined) {
    return;
  }

  popup.parentNode.removeChild(popup);

The comparisons can both be swapped around for simpler and shorter code:

    if (validateTitle(field)) {
      clearWarning();
    }
    else {
      showWarning();
    }
  if (popup) {
    popup.parentNode.removeChild(popup);
  }
\$\endgroup\$
2
  • 3
    \$\begingroup\$ Because "use strict"; silently fails if it's in the wrong place, putting it at the beginning of a function is “safer” than putting it at file level where it might be concatenated or edited into not being at the top any more (especially if it is e.g. preceded by comments). But you can put one big function around the whole program, i.e. ({ "use strict"; ... }());, which both avoids the repetition and avoids the possibility of forgetting to put it in an individual function. \$\endgroup\$
    – Kevin Reid
    Commented Jun 12, 2015 at 23:36
  • \$\begingroup\$ Also "use strict" at the top of your file can mess up other code that isn't written to operate in strict mode and may be included after the above. \$\endgroup\$
    – sunny
    Commented Oct 1, 2015 at 12:33
3
\$\begingroup\$

It would be great to stop the form from submitting when the warning is displayed:

if (!validateTitle(field)) {
    showWarning();

Add a call to the preventDefault method:

    e.preventDefault();

In the snippet below, I added it in the entire block where ENTER is pressed so the form will not be submitted, as that doesn't make much sense in a Stack snippet.


It is best to avoid implicit type coercions and use strict equality operators like === and !==. The problem with loose comparisons (e.g. ==, !=) is that it has so many weird rules one would need to memorize in order to be confident in its proper usage.

So this line:

if (e.keyCode == 13) {

can be updated to

 if (e.keyCode === 13) {

Currently in KeyboardEvent.keyCode is deprecated 1 and KeybordEvent.key is supported in all major browsers plus IE 9+ 2. So that condition above can be updated to:

if (e.key === 'Enter') {

function validateKey(e) {
  "use strict";
  if (e.key === 'Enter') {
    var field = document.getElementById("title");
    if (!validateTitle(field)) {
      showWarning();
    }
    else {
      clearWarning();
    }
    e.preventDefault();
  } 
}


function validateTitle(titleField) {
  "use strict";
  var title = titleField.value;
  return !title.includes("bug") 
      && !title.includes("issue") 
      && !title.includes("t work") // catches won't work, isn't working, etc.
      && !title.includes("s wrong") // catches what's wrong, what is wrong
      && !title.includes("fix")
      && !title.includes("why");
}


function showWarning() {
  "use strict";
  var msg = getWarningDiv();
  document.getElementsByTagName("table")[0].appendChild(msg);
}

function clearWarning() {
  "use strict";
  var popup = document.getElementsByClassName("message-dismissable")[0];
  if (popup == undefined) {
    return;
  }
  
  popup.parentNode.removeChild(popup);
}

function getWarningDiv() {
  "use strict";
  
  var popup = document.createElement("div");  
  popup.className = "message message-error message-dismissable";
  popup.setAttribute("style", "max-width: 270px; min-width: 270px; position: absolute; top: 32px; left: 378px; display: block;");
  popup.addEventListener("click", function(event) { clearWarning(); });
  
  var tooltip = document.createElement("div");
  tooltip.className = "message-inner message-tip message-tip-left-top";
  
  popup.appendChild(tooltip);
  
  var tipText = document.createElement("div");
  tipText.title = "close this message (or hit Esc)";
  tipText.className = "message-close";
  tipText.textContent  = "×";
  
  tooltip.appendChild(tipText);
  
  var msg = document.createElement("div");
  msg.className = "message-text";
  msg.setAttribute("style", "padding-right: 35px;");
  msg.textContent = "Wait! If your code does not work as intended, this question is off-topic!";
  
  popup.appendChild(msg);
  return popup;
}
body {
  font-family: 'Helvetica Neue',Helvetica,Arial,sans-serif;
  font-size: 13px;
  line-height: 1.3em;
  color: #222;
  background: #fff;
  min-width: 1030px;
}

.ask-title-table {
  width: 668px;
  height: 44px;
}

table {
  border-collapse: collapse;
  border-spacing: 0;
  border-color: grey;
}

.ask-title-cell-key {
  width: 40px;
}

#title {
  width: 498px;
}

.form-item label {
  display: block;
  font-weight: bold;
  padding-bottom: 3px;
}

input[type=text], input[type=url], input[type=email], input[type=tel], textarea {
  font-family: 'Helvetica Neue',Helvetica,Arial,sans-serif;
  background: #fff;
  color: #444;
  border: 1px solid #ccc;
  font-size: 14px;
  padding: 8px 10px;
}

.message.message-error.message-dismissable {
  cursor: pointer;
}

.message.message-error {
  z-index: 1;
  display: none;
  color: #fff;
  background-color: #c04848;
  text-align: left;
}

.message.message-error .message-text {
  padding: 15px;
}

.message.message-error .message-close {
  padding: 2px 6px 3px 6px;
  font-family: Arial,sans-serif;
  font-size: 16px;
  font-weight: normal;
  color: #fcb2b1 !important;
  line-height: 1;
  float: right;
  border: 1px solid rgba(255,255,255,0.2);
  margin-top: 8px;
  margin-right: 8px;
}

.message.message-error .message-tip-left-top:before {
  top: 0;
  left: -9px;
  border-top: 9px solid #c04848;
  border-left: 9px solid transparent;
}

.message.message-error .message-tip:before {
  content: "";
  position: absolute;
}
<script src="https://ajax.googleapis.com/ajax/libs/jquery/2.1.1/jquery.min.js"></script>
<html>
  <head>
      <title>Ask a Question - Code Review Stack Exchange</title>
  </head>
  <body>
    <form>
    <div id="question-form">
      <div class="form-item ask-title">
        <table class="ask-title-table">
          <tbody>
            <tr>
              <td class="ask-title-cell-key">
                <label for="title">Title</label>
              </td>
              <td class="ask-title-cell-value">
                <input id="title" name="title" type="text" maxlength="300" tabindex="100" placeholder="State the task that your code accomplishes. Make your title distinctive." class="ask-title-field" data-min-length="15" data-max-length="150" autocomplete="off" onkeypress="return validateKey(event)"/>
              </td>
            </tr>
          </tbody>
        </table>
      </div>
      <!-- warning should be inserted here -->
    </div>
    </form>
  </body>
</html>

1https://developer.mozilla.org/en-US/docs/Web/API/KeyboardEvent/keyCode)

2https://developer.mozilla.org/en-US/docs/Web/API/KeyboardEvent/key#browser_compatibility)

\$\endgroup\$

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.