9
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ I think current title is too similar to the first review request on this code. The code is very different, but the purpose is generally the same. Suggestions on title edit? \$\endgroup\$ – Viziionary Jun 30 '14 at 21:49
  • 2
    \$\begingroup\$ I see no problem with having similar titles since those scripts do the same things and it's a follow up. I would re-add the same tags. \$\endgroup\$ – Marc-Andre Jun 30 '14 at 21:53
  • \$\begingroup\$ Uh oh. I ended up not using the newline variable. Woops. \$\endgroup\$ – Viziionary Jun 30 '14 at 22:42
  • \$\begingroup\$ The coding patterns section that I just added to my question might be difficult to apply with the relatively small amount of code. It's just what I'm focusing on with my JS coding practices right now, and I thought maybe worth mentioning. \$\endgroup\$ – Viziionary Jul 2 '14 at 1:29
7
\$\begingroup\$

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,
\$\endgroup\$
  • \$\begingroup\$ Thank you, I've noted all suggestions. I'm not sure what the best practice is on variable declarations, but I like your way personally. Just out of curiosity, what browser are you using? That weird insertion doesn't happen in Chrome. \$\endgroup\$ – Viziionary Jul 20 '14 at 22:22
  • \$\begingroup\$ @jt0dd I'm using Firefox 30.0 \$\endgroup\$ – Simon Forsberg Jul 20 '14 at 23:16
4
\$\begingroup\$

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.

\$\endgroup\$
  • 1
    \$\begingroup\$ There is an alternative (only one alternative that fits, considering Windows shortcuts): Ctrl + Spacebar with Shift + Ctrl + Spacebar for reverse. \$\endgroup\$ – Viziionary Jul 2 '14 at 10:05
  • \$\begingroup\$ You accidentally wrote shift + enter rather than Spacebar \$\endgroup\$ – Viziionary Jul 2 '14 at 10:06
  • \$\begingroup\$ @jt0dd Nice catch with the Enter. Was rather in a hurry to explain. Also, why space when you can just override Tab? Most IDEs use Tab, jsfiddle and jsbin also use Tab. \$\endgroup\$ – Joseph Jul 2 '14 at 12:21
  • 1
    \$\begingroup\$ That request (override tab) was declined by SE. Oded, a developer in charge of such things pointed out that overriding tab would break tab order, a native browser functionality. people with screen readers rely on tab to move from element to element. \$\endgroup\$ – Viziionary Jul 2 '14 at 16:59
  • \$\begingroup\$ +1 for the pattern example though. I've appended it as an alternative within the feature request. \$\endgroup\$ – Viziionary Jul 2 '14 at 18:57

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.