Problem
I came up with an idea for a userscript to simplify writing answers on Stack Exchange while writing a previous answer.
This userscript adds “Review”-links on the top of each code segment. When clicked, it adds checkboxes in front of every code line. You select the checkboxes for the lines you want to comment about, click the link again – which has now changed to “Add to answer”, and voilà – the code will be copied to your answer.
I believe this userscript can significantly reduce the scrolling up and down where you consistently copy some code from the question, paste it in the answer, scroll up again, copy code, scroll down, paste, scroll up copy, etc… Now you can just go through the code from top to bottom and select the lines you want to comment on.
I have tested this userscript using Google Chrome + Tampermonkey, but I believe it should also work with Firefox + Greasemonkey, and with other browser/extension combinations that support userscripts.
The code is also available on GitHub.
If you are able to include a userscript from a link, use this link.
// ==UserScript==
// @name Auto-Review
// @author Simon Forsberg
// @namespace zomis
// @homepage https://www.github.com/Zomis/Auto-Review
// @description Adds checkboxes for copying code in a post to an answer.
// @include http://stackoverflow.com/*
// @include http://meta.stackoverflow.com/*
// @include http://superuser.com/*
// @include http://serverfault.com/*
// @include http://meta.superuser.com/*
// @include http://meta.serverfault.com/*
// @include http://stackapps.com/*
// @include http://askubuntu.com/*
// @include http://*.stackexchange.com/*
// @exclude http://chat.stackexchange.com/*
// ==/UserScript==
function embedFunction(name, theFunction) {
var script = document.createElement('script');
script.type = 'text/javascript';
script.textContent = theFunction.toString().replace(/function ?/, 'function ' + name);
document.getElementsByTagName('head')[0].appendChild(script);
}
embedFunction('showAutoreviewButtons', function(clickedObject) {
var i;
if ($(clickedObject).data('review')) {
var answer = $("#wmd-input");
var answer_text = answer.val();
var added_lines = 0;
var added_blocks = 0;
// loop through checkboxes and prepare answer
var checkboxes = $("input.autoreview");
var block = [];
for (i = 0; i < checkboxes.length; i++) {
if (!$(checkboxes[i]).prop('checked')) {
continue;
}
var checkbox = $(checkboxes[i]);
var line_data = (checkbox).data('line');
block.push(line_data);
if ((i === checkboxes.length - 1) || !$(checkboxes[i + 1]).prop('checked')) {
// add block
var block_line;
var cut_count = 1000;
for (block_line = 0; block_line < block.length; block_line++) {
var cut_this = block[block_line].indexOf(block[block_line].trim());
if (cut_count > cut_this) {
cut_count = cut_this;
}
}
for (block_line = 0; block_line < block.length; block_line++) {
answer_text = answer_text + "n " + block[block_line].substr(cut_count);
}
answer_text += "nn---n";
added_lines += block.length;
added_blocks++;
block = [];
}
}
answer.val(answer_text);
alert(added_lines + " lines in " + added_blocks + " blocks added to answer.");
return;
}
$(clickedObject).data('review', true);
$(clickedObject).text("Add to answer");
var spans = $("code span", $(clickedObject).next());
console.log(spans.length);
var count = spans.length;
var line = "";
var first = null;
for (i = 0; i < count; i++) {
var element = $(spans[i]);
if (first === null) {
first = element;
}
if (element.text().indexOf("n") !== -1) {
console.log(i + " line: " + line);
var lines = element.text().split("n");
element.text("");
for (var line_index = 1; line_index < lines.length; line_index++) {
var current_line = lines[line_index];
var prev_line = lines[line_index - 1];
var span;
// Add the last part of the previous line
if (line_index == 1) {
line += prev_line;
span = $('<span class="pln zomis before">' + prev_line + 'n</span>');
element.after(span);
element = span;
}
// Add the checkbox for the previous line
if (line.length > 0) {
var dataProperty = 'data-line="' + line + '" ';
var checkbox = $('<input type="checkbox" ' + dataProperty + ' class="autoreview"></input>');
first.before(checkbox);
first = null;
}
// Add the beginning <span> element for the current line
if (line_index < lines.length - 1) {
current_line += "n";
}
span = $('<span class="pln zomis after">' + current_line + '</span>');
element.after(span);
first = span;
element = span;
line = current_line;
}
}
else {
line += element.text().replace(/\/g, '\\').replace(/"/g, '\"');
}
}
if (line.length > 0) {
dataProperty = 'data-line="' + line + '" ';
checkbox = $('<input type="checkbox" ' + dataProperty + ' class="autoreview"></input>');
first.before(checkbox);
}
});
$('pre code').parent().before('<a href="javascript:void(0);" onclick="showAutoreviewButtons($(this))">(Review)</a>');
Primary questions:
- Am I following JavaScript conventions? I have seen
people.coding( "like this" );with extra spaces around each parameter, and I’ve also seen people declaring all variables at the beginning of a method. Is this some official convention? Am I break any official convention here? I feel that I am using Java conventions in my JavaScript code. - Can the code be cleaned up, and utility methods and stuff be used, while still keeping browser compatibility? For example, in the code to figure out the indentation for a block I was thinking of how I would do it in Java, where I would use
block.stream().mapToInt(str -> str.indexOf(str.trim())).min(), can something similar be done here? - Any other comments welcome!
I bet there are many things that can be improved. The fact that it’s a userscript made me feel a bit restricted on what I could do, but perhaps that’s just because I am not aware of how to do them for userscripts.
I plan on posting this userscript to http://www.stackapps.com when I’m happy enough with it, any feature-requests / UI suggestions can be added as issues on github, or you can ping me in The 2nd Monitor.
Auto-upvote to anyone who is using the script while writing their answer!
Solution
Naming
I realize naming can be hard, but ‘Auto-Review’ is a name that conflicts a bit with the exiting UserScript on StackExchange called AutoReviewComments
Protocols
There are a number of UserScript conventions you are failing to adhere to.
First up, for Firefox, you need to wrap the header section in to a “preserved” comment block:
/** @preserve
// ==UserScript==
.....
// ==/UserScript==
*/
The preserve is required to keep the comment block after the javascript is processed. This allows the ‘compiled’ version to keep the references, and for FireFox to know what it is about still.
Other browsers may not have the same requirements.
Additionally, you need to specify @grant permissions too for GreaseMonkey to be happy. In your case, none is appropriate:
// @grant none
Once I made those modifications the userscript loaded up nicely in my FireFox.
Usability
There are four user-experience enhancements that I would suggest:
- NO POPUPS – popups are a horrible distraction
- Scroll-to-inserted-content – after inserting the code blocks, scroll to the edit point and make it visible on the screen
- Trigger a changed-event on the answer entry box – this will update the ‘preview’ of the answer. As it is currently, you have to manually change something in the answer box for the preview to update.
- You should clear the checkboxes after processing them. Having to un-check each box is a PITA if you need to copy different blocks later.
Review
-
double-array-dereferencing is a micro-optimization that’s unnecessary. You have the following code (copied here using your userscript):
for (i = 0; i < checkboxes.length; i++) { if (!$(checkboxes[i]).prop('checked')) { continue; } var checkbox = $(checkboxes[i]); var line_data = (checkbox).data('line');That code double-dereferences
$(checkboxes[i]). I presume this is because you don’t want to carry the overhead of the variable if it is not checked. This is an early-optimization. The code would be simpler as:for (i = 0; i < checkboxes.length; i++) { var checkbox = $(checkboxes[i]); if (!checkbox.prop('checked')) { continue; } var line_data = (checkbox).data('line'); -
vardeclarations. JavaScript ‘hoists’ all variable declarations to the top of the function that contains it. Unlike other languages, JavaScript should not be coded with ‘block-local’ declarations. It is best practice to move all variable declarations to be the first items in the function. This Stack Overflow answer does a better job of explaining it than I would.
Coding style
I’m not a JavaScript guru,
but I’m pretty sure the coding style guide is much less strict compared to Java,
for example.
The way used spacing around operators,
and the placement of braces like in Java seem very common,
and I’ve never heard anybody object to that.
There are some other styles like the ones you mentioned,
but they are rare.
Some people also prefer indentation using 2 spaces instead of 4,
but it seems to be largely a matter of taste.
For reference,
PyCharm is a fantastic IDE for Python + Web development by JetBrains (same breed as IntelliJ),
it has good support for JavaScript,
and it doesn’t object to your code in terms of style.
I’d take that as a good sign.
For code like people.coding( "like this" );,
the auto-format function removes the spaces inside the parens.
Bad practices
First of all, paste the code in http://jshint.com/ and see for yourself:
- One duplicate variable definition:
checkbox - A few variables used out of scope
In addition,
I find it odd that you don’t take full advantage of jQuery in embedFunction.
Since you already use jQuery quite a bit elsewhere in the script,
for example instead of:
document.getElementsByTagName('head')[0].appendChild(script);
You could simplify as:
$('head').append(script);
UPDATE: It seems (from your comment) that this suggestion doesn’t work for you “as-is”. I’ll try to figure out why. In the meantime, my objection still stands: it’s a bit odd to have a mixture of classic JavaScript and jQuery within the same script, as if it was done by two different people or the same person at different times. I think it’s better to be consistent and write either classic JavaScript without depending on jQuery, or fully embrace jQuery.
Cache the result of $(...) lookups
DOM lookups are not exactly cheap.
So whenever you repeat a lookup,
consider caching in a variable instead, for example here:
if (!$(checkboxes[i]).prop('checked')) {
continue;
}
var checkbox = $(checkboxes[i]);
The above is better if you first assign to checkbox, and then do the if.
It also reduces the duplication of checkboxes[i].
Later in the code I see $(clickedObject) many times.
It would be good to cache that too, of course.
Strange elements
On this line:
var line_data = (checkbox).data('line');
The parens around checkbox are… curious…
You don’t need them,
and might confuse the reader that it was intended as jQuery with mistakenly omitted $ (which it is not).
Augmented assignment
This assignment can be replaced with += augmented assignment:
answer_text = answer_text + "n " + block[block_line].substr(cut_count);
I know this code has been around for ~5 years but still appears to work properly despite some DOM changes on these sites. You no doubt have learned a lot since posting this but I feel there are other things that haven’t been mentioned yet/incorporated into the github script that could improve/simplify the code.
Long functions
I recently have become accustomed to the github style UI of selecting lines to comment on by click and drag selection. I considered making such a userscript and might utilize some of this code. I started tinkering with it and noticed it has a lot of code in one giant function, as well as some large loops. I would recommend using separate functions for the event handlers- e.g. one for clicks on the buttons to add checkboxes, and another for clicks on the buttons to add checked lines to reviews. I know it isn’t exactly a direct application of the Single Responsibility Principle but feels related.
finding checked checkboxes
The jQuery :checked selector can simplify the code to find
checkboxes when copying to answer- from:
var checkboxes = $("input.autoreview");
for (i = 0; i < checkboxes.length; i++) {
if (!$(checkboxes[i]).prop('checked')) {
continue;
}
var checkbox = $(checkboxes[i]);
to this:
var checkboxes = $("input.autoreview:checked");
for (i = 0; i < checkboxes.length; i++) {
var checkbox = $(checkboxes[i]);
A functional approach could be used instead using Array.reduce(). jQuery does have similar functional methods like the filtering methods – e.g. .map() and .each() which are similar to the native array methods like Array.map() though note the parameter differences between the jQuery equivalents.
Primary Question 2
For example, in the code to figure out the indentation for a block I was thinking of how I would do it in Java, where I would use
block.stream().mapToInt(str -> str.indexOf(str.trim())).min(), can something similar be done here?
With ecmascript-6 features things like dereferencing arrays can be simplified- i.e. using a for...of loop. And after reviewing your Royal game of Ur code I know you are familiar with arrow functions. Those can be used with functional methods like Array.map(), Array.filter(), Array.reduce() to simplify the syntax.
Update Apr. 27, 2020
PR #4 created and merged incorporating many suggestions from above.