Script to convert Shift + Spacebar into four spaces on SE - Second Improvement

Question

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.

Answer 1

Overall, your code looks nice and clean, I just have a few things to say:

Your code is described as "... convert ... into four spaces" which throws me a bit off pace when reading this line:

for (var i = 1; i < 5; i++) {

When reading this I first thought "Where did that five come from?" but of course it does only loop four times. However, I would prefer:

for (var i = 1; i <= 4; i++) {

or:

for (var i = 0; i < 4; i++) {

---

I am not completely aware of the best practices in JavaScript, but I would personally prefer one variable declaration per line, which would be:

 var pad = "    ";
 var text = this.value;
 var start = this.selectionStart;

---

I tested your JSFiddle and I selected a couple of lines, thinking that it would add four spaces to each line when I pressed Shift + Space. However, it seems like the selected code got duplicated after pressing Shift + Space.

I would expect selecting this part:

$("textarea").keydown(function (e) {
    if (e.shiftKey && e.keyCode === 32) {
        var pad = "    ",
            tex

To result in this:

    $("textarea").keydown(function (e) {
        if (e.shiftKey && e.keyCode === 32) {
            var pad = "    ",
                text = this.value,
            start = this.selectionStart,
            end = this.selectionEnd,

That is, four spaces being added to each of the selected lines.

This is just my preference though, but either way, I believe that the actual result below was not part of the plan:

    $("textarea").keydown(function (e) {
        if (e.shiftKey && e.keyCode === 32) {
            var pad = "    ",
                tex$("textarea").keydown(function (e) {
    if (e.shiftKey && e.keyCode === 32) {
        var pad = "    ",
            text = this.value,
            start = this.selectionStart,
            end = this.selectionEnd,

Answer 2

This answer is not a full review, but just a minor tweak.

So you go by Shift+Space to indent, and Ctrl+Shift+Space to outdent. That's really weird. Take a moment to observe what Shift actually does in common keyboard shortcuts:

Page Scrolling

• Space to scroll down the web page
• Shift+Space to scroll back up (It's a conflict. Besides that, there's the point I'm getting to).

IDE indention

• Tab to indent
• Shift+Tab to outdent

Cycling through browser tabs

• Ctrl+Tab to cycle forward
• Shift+Ctrl+Tab to cycle backward

Cycling through open windows on desktop

• Alt+Tab to cycle forward
• Shift+Alt+Tab to cycle backward

You do notice the pattern right? Shift generally reverses the action. So I'd think twice using Shift that way. Sure, it's "custom shortcut". However, you'd have to check convention as well, especially that if this feature is non-modifiable.

I'd stick to hijacking Tab instead.