Return to Answer

• 1

• 1

• 416
• 2
• 14

A quote from another answer of mine:

You don't use quotes or apostrophes for string literals consistently. Decide if you want to write string using either apostrophes or double quotes and use it consistently in the whole code. Personally, I prefer apostrophes, because they require to press only one key, whereas to insert a double quote character you have to hold Shift too.

And for example if you choose apostrophes and you want to make a string which contains apostrophes, feel free to use double quotes in this specific situation, so you don't have to escape the apostrophes with a backslash.

And from yet another:

You're not using the const keyword anywhere. It's a good practice to use const instead of var always when possible, that is, when you don't intend to reassign the variable. It can prevent you from accidentally reassigning some other value to the variable, because it throws an error if you try to (at least in strict mode).

And from yet another:

Oh come on, it's 21. century! Why do you still use .onevent properties? You should totally drop that and try (削除) jQuery (削除ここまで) .addEventListener(). See addEventListener vs onclick.

Always use three-equals operator (===) for equality comparisons. See Does it matter which equals operator (== vs ===) I use in JavaScript comparisons?.

Instead of using the concat() method you could use the spread syntax:

const titleRules = [...sumRules, /(\d)1円{2}/, /care\b/i, /\bwatch\b/i];

You could replace most of your regular functions with arrow functions.

var urlRules = []; // Will be loaded from the SmokeDetector github

$.ajax("https://cdn.rawgit.com/Charcoal-SE/SmokeDetector/master/blacklisted_websites.txt",{

 cache: true,
 success: function(e)
 {
 urlRules = e
 .filter(function(i){return i;})
 .map(function(i){try{return new RegExp(i, "i");}catch(e){}})
 .filter(function(i){return !!i;});
 },
 dataType: "text",
 dataFilter: function(i){return i.split("\n");}
});

A few notes about this code:

• You could change the IIFE to use an async function (i.e. change the (function() { at the top to (async function() {) and use the await operator here. This way the rest of the code would wait before this request completes, which would prevent bugs when the request would be taking a long time.
• The cache option is true by default, so there's no need to set it to true.
• dataType: 'text' is also redundant, because the resource returns a correct Content-Type header.
• If the purpose of .filter(function(i){return i;}) is to filter out empty lines, you should be more explicit about that. You could do it like that: .filter(x => x !== '').
• What's the point of the try...catch statement? The RegExp constructor shouldn't throw any errors.
• Again, .filter(function(i){return !!i;}) is rather ambiguous. It filters out elements which return false when converted to boolean. Since all the elements are RegExp instances, this should never happen.

I would rewrite this code fragment like that:

const urlRules = (await $.get('https://cdn.rawgit.com/Charcoal-SE/SmokeDetector/master/blacklisted_websites.txt'))
 .split('\n')
 .filter(x => x !== '')
 .map(x => new RegExp(x, 'i'));

If you want to create multiline strings, you can use template literals.

Why are you using native DOM methods like document.getElementById() when you already have jQuery?

var message = messageList[messageList.length-1];

It would more readable if you inserted spaces before and after the minus, like that:

const message = messageList[messageList.length - 1];

wsVolume = wsVolume + e.data.length;

You could use the += operator here.

There are probably many other issues with this code, but I think it's enough for one time.