Secondary Review
Changes:
I recently requested a review: Script to convert Shift + Spacebar into four spaces on SE. After accepting suggestions and improvements by David Harkness, I heavily updated my code to include his suggestions, improvements, and other revisions. The code is now very different from the originally suggested improvement, and I would like a review of my new code.
Purpose:
The purpose of this code is primarily to implement Shift + Spacebar as a "tab" shortcut within Stack Exchange sites (potentially any text area). This version of the code additionally supports out-denting with Shift + Ctrl + Spacebar, multi-line selections, and avoids accidental usage when applying a space after shifting for a capital letter. JS Fiddle Here.
Code:
$("textarea").keydown(function (e) {
if (e.shiftKey && e.keyCode === 32) {
var pad = " ",
text = this.value,
start = this.selectionStart,
end = this.selectionEnd,
scroll = this.scrollTop,
before = text.slice(0, start),
after = text.slice(start),
selection = text.slice(start, end),
newline = /\r|\n/.exec(selection),
prevChar = before.charAt(before.length - 1),
prevFourChar = "",
beforeLess = before.slice(0, start - 4);
if (e.ctrlKey) {
for (var i = 1; i < 5; i++) {
prevFourChar += before.charAt(before.length - i);
// Check the selection's previous 4 characters.
// If they're important, don't overwrite them.
// Maybe a better alternative: Include any previous
// non-space characters on that line in the tab's
// effect.
}
if (prevFourChar === pad) {
selection = selection.split("\n" + pad).join("\n");
this.value = beforeLess + selection + after;
this.selectionStart = start - pad.length;
this.selectionEnd = end - pad.length;
}
} else if (prevChar === "" || prevChar === " " || prevChar === "\n") {
selection = pad + selection.split("\n").join("\n" + pad);
this.value = before + selection + after;
this.selectionStart = start + pad.length;
this.selectionEnd = end + pad.length;
}
this.scrollTop = scroll;
e.preventDefault();
}
});
I'm looking for:
- general improvements
- best practices
- simplification.
Additional improvement:
I have been trying to to condense some of the code that is a near duplicate of itself within the script:
Compare
selection = selection.split("\n" + pad).join("\n"); this.value = beforeLess + selection + after; this.selectionStart = start - pad.length; this.selectionEnd = end - pad.length;
To
selection = pad + selection.split("\n").join("\n" + pad); this.value = before + selection + after; this.selectionStart = start + pad.length; this.selectionEnd = end + pad.length;
Update:
The attempt to combine the functionality into a single function was more or less successful, however it required significantly more code than simply using the semi-duplicate pieces did.
So I'm wondering:
When should I condense duplicate code into a function?
Is the answer simply "when the outcome is shorter and more readable."?
What if the outcome is shorter, but not quite as readable or "follow-able"?
JavaScript coding patterns I'd like to focus on for this code:
I recently began reading about coding patterns that apply to JavaScript, and have decided to begin practicing many of these patterns where applicable to improve my code. Feel free to focus on which of the patterns I didn't but should have utilized and my success with the utilization of ones that I did.
Single Function Function:
Instead of having a huge mess of callback hell, keep a single function to a single task, and do that task well. Sometimes you can get ahead of yourself and add more functionality within each function, but ask yourself: Can this become an independent function? Name the function, and this cleans up your indentation and, as a result, cleans up the callback hell problem.
While intended here to solve the issue of callbacks, I see value in the idea of limiting functions to a single specific purpose based on the Single Responsibility Principle.
I didn't use this principle here, but perhaps I should have. I packed everything into that single anonymous function call. What if I wanted to expand on this feature? It might become a problem.
newline
variable. Woops. \$\endgroup\$ – J.Todd Jun 30 '14 at 22:42