12
\$\begingroup\$

I recently joined this GitHub project based on a user script that offers a powerful editing tool to Stack Exchange Editors.

enter image description here

My Goal:

In the past few days, I've spent many hours improving the code, but today I re-wrote the whole thing into a completely new infrastructure. Here was my objective as written in a discussion on the repo:

Right now the method in which editing is done is sort of jumping in and out of functions. This is a note to convert the entire project into more of a pipeline infrastructure. This is important for exensibility; We decide to add something new (like me working on spell checking), we just add a layer to the pipeline that grabs / modifies the content at some point on it's way to being output, and flows it right back into the line for further processing.

Mini Model:

Here is the working concept model I built to try it before implementing:

//overwrite console log just to output into stack snippets (horrid practice)
console.log = function (input){
    document.body.innerHTML += (input + "<br/>");
}

//define our namespace
var App = {};

//place item data in one place
//this keeps in mind multi question inline editing support
App.items = [{
    title: 'title text here',
    body: 'body here',
    summary: 'summary will go here'
}, {
    //second edit item if multiple on page and so on
}];

//dummy plain string data to make this work in a test
var dummyData = ("I'm just a string, but I should be an object containing sets of edit items.");

//define modules in one place
App.pipeMods = {
    edit: function (data) {
        return (data + " Edited!");
    },
    omitCode: function (data) {
        return (data + " Code omitted!")
    },
    checkSpelling: function (data) {
        return (data + " Spelling Checked!")
    }
}

//define order in which mods affect data
App.order = [
    "omitCode",
    "edit",
    "checkSpelling"];

//wa-la sexy, simple, extendable code infrastructure:
App.pipe = function (data, mods, order) {
    var modName;
    for (var i in order) {
        modName = order[i];
        data = mods[modName](data);
    }
    return data;
}

console.log(App.pipe(dummyData, App.pipeMods, App.order));

The following code is indeed code created by multiple contributors, but I have edited and changed it so heavily that it is 95% different code, so I hope I can receive a review on it regardless. I am looking for critique on the way in which I handled variable scope, placement, etc, and of course, whatever advice you have to give.

// ==UserScript==
// @name           Stack-Exchange-Editor-Toolkit
// @author         Cameron Bernhardt (AstroCB)
// @developer      jt0dd
// @contributor    Unihedron
// @namespace  http://github.com/AstroCB
// @version        3.0
// @description  Fix common grammar/usage annoyances on Stack Exchange posts with a click
// @include        http://*.stackexchange.com/questions/*
// @include        http://stackoverflow.com/questions/*
// @include        http://meta.stackoverflow.com/questions/*
// @include        http://serverfault.com/questions/*
// @include        http://meta.serverfault.com/questions/*
// @include        http://superuser.com/questions/*
// @include        http://meta.superuser.com/questions/*
// @include        http://askubuntu.com/questions/*
// @include        http://meta.askubuntu.com/questions/*
// @include        http://stackapps.com/questions/*
// @include        http://*.stackexchange.com/posts/*
// @include        http://stackoverflow.com/posts/*
// @include        http://meta.stackoverflow.com/posts/*
// @include        http://serverfault.com/posts/*
// @include        http://meta.serverfault.com/posts/*
// @include        http://superuser.com/posts/*
// @include        http://meta.superuser.com/posts/*
// @include        http://askubuntu.com/posts/*
// @include        http://meta.askubuntu.com/posts/*
// @include        http://stackapps.com/posts/*
// @exclude        http://*.stackexchange.com/questions/tagged/*
// @exclude        http://stackoverflow.com/questions/tagged/*
// @exclude        http://meta.stackoverflow.com/questions/tagged/*
// @exclude        http://serverfault.com/questions/tagged/*
// @exclude        http://meta.serverfault.com/questions/*
// @exclude        http://superuser.com/questions/tagged/*
// @exclude        http://meta.superuser.com/questions/tagged/*
// @exclude        http://askubuntu.com/questions/tagged/*
// @exclude        http://meta.askubuntu.com/questions/tagged/*
// @exclude        http://stackapps.com/questions/tagged/*
// ==/UserScript==
var main = function () {

/*

 Note that in the new version I place many things needlessly into wrappers (container
 functions) and namespaces (container variables); this is simply an effort to promote modularity
 in the structure and keep focused on what's going where and when.

 Some of this may have no use at all once the code is all in place, and we may be able to simplify it
 extensively. This is one of my first user-script projects, and it's confusing putting
 so many different functionalities into one single file.

 */

    // Define app namespace
    var App = {};

    // Place edit items here
    App.items = [];

    // Place selected JQuery items here
    App.selections = {};

    // Place "global" app data here
    App.globals = {};

    // Place "helper" functions here
    App.funcs = {};

    //Preload icon alt

    var SEETicon = new Image();

    SEETicon.src = 'http://i.imgur.com/d5ZL09o.png';

    // Populate global data

    // Get url for question id used in id and class names
    App.globals.URL = window.location.href;

    // Get question num from URL
    App.globals.questionNum = App.globals.URL.match(/\d/g);

    // Join
    App.globals.questionNum = App.globals.questionNum.join("");

    // Define varables for later use
    App.globals.barReady = false;
    App.globals.editsMade = false;
    App.globals.editCount = 0;
    App.globals.infoContent = '';

    App.globals.privileges = true;
    App.globals.spacerHTML = '<li class="wmd-spacer wmd-spacer3" id="wmd-spacer3-' + App.globals.questionNum + '" style="left: 400px !important;"></li>';
    App.globals.buttonHTML = '<div id="ToolkitButtonWrapper"><button class="wmd-button" id="ToolkitFix"></button><div id="ToolkitInfo"></div></div>';

    App.globals.reasons = [];
    App.globals.numReasons = 0;

    App.globals.replacedStrings = {
        "block": [],
            "inline": []
    };
    App.globals.placeHolders = {
        "block": "_xCodexBlockxPlacexHolderx_",
            "inline": "_xCodexInlinexPlacexHolderx_"
    };
    App.globals.checks = {
        "block": /(    )+.*/gm,
            "inline": /`.*`/gm
    };

    // Assign modules here
    App.globals.pipeMods = {};

    // Define order in which mods affect  here
    App.globals.order = [
        "omit",
        "edit",
        "replace"];


    // Define edit rules
    App.edits = {
        i: {
            expr: /(^|\s|\()i(\s|,|\.|!|\?|;|\/|\)|'|$)/gm,
            replacement: "$1I$2",
            reason: "basic capitalization"
        },
        so: {
            expr: /(^|\s)[Ss]tack\s*overflow|StackOverflow(.|$)/gm,
            replacement: "$1Stack Overflow$2",
            reason: "'Stack Overflow' is the proper capitalization"
        },
        se: {
            expr: /(^|\s)[Ss]tack\s*exchange|StackExchange(.|$)/gm,
            replacement: "$1Stack Exchange$2",
            reason: "'Stack Exchange' is the proper capitalization"
        },
        expansionSO: {
            expr: /(^|\s)SO(\s|,|\.|!|\?|;|\/|\)|$)/gm,
            replacement: "$1Stack Overflow$2",
            reason: "'SO' expansion"
        },
        expansionSE: {
            expr: /(^|\s)SE(\s|,|\.|!|\?|;|\/|\)|$)/gm,
            replacement: "$1Stack Exchange$2",
            reason: "'SE' expansion"
        },
        javascript: {
            expr: /(^|\s)[Jj]ava\s*script(.|$)/gm,
            replacement: "$1JavaScript$2",
            reason: "'JavaScript' is the proper capitalization"
        },
        jsfiddle: {
            expr: /(^|\s)[Jj][Ss][Ff]iddle(.|$)/gm,
            replacement: "$1JSFiddle$2",
            reason: "'JSFiddle' is the currently accepted capitalization"
        },
        caps: {
            expr: /^(?!https?)([a-z])/gm,
            replacement: "$1",
            reason: "basic capitalization"
        },
        jquery: {
            expr: /(^|\s)[Jj][Qq]uery(.|$)/gm,
            replacement: "$1jQuery$2",
            reason: "'jQuery' is the proper capitalization"
        },
        html: {
            expr: /(^|\s)[Hh]tml(?:5*)(\s|$)/gm,
            replacement: "$1HTML$2",
            reason: "HTML: HyperText Markup Language"
        },
        css: {
            expr: /(^|\s)[Cc]ss(\s|$)/gm,
            replacement: "$1CSS$2",
            reason: "CSS: Cascading Style Sheets"
        },
        json: {
            expr: /(^|\s)[Jj]son(\s|$)/gm,
            replacement: "$1JSON$2",
            reason: "JSON: JavaScript Object Notation"
        },
        ajax: {
            expr: /(^|\s)[Aa]jax(\s|$)/gm,
            replacement: "$1AJAX$2",
            reason: "AJAX: Asynchronous JavaScript and XML"
        },
        angular: {
            expr: /[Aa]ngular[Jj][Ss]/g,
            replacement: "AngularJS",
            reason: "'AngularJS is the proper capitalization"
        },
        thanks: {
            expr: /(thanks|please\s+help|cheers|regards|thx|thank\s+you|my\s+first\s+question).*$/gmi,
            replacement: "",
            reason: "'$1' is unnecessary noise"
        },
        commas: {
            expr: /,([^\s])/g,
            replacement: ", $1",
            reason: "punctuation & spacing"
        },
        php: {
            expr: /(^|\s)[Pp]hp(\s|$)/gm,
            replacement: "$1PHP$2",
            reason: "PHP: PHP: Hypertext Preprocessor"
        },
        hello: {
            expr: /(?:^|\s)(hi\s+guys|good\s(?:evening|morning|day|afternoon))(?:\.|!)/gmi,
            replacement: "",
            reason: "Greetings like '$1' are unnecessary noise"
        },
        edit: {
            expr: /(?:^\**)(edit|update):?(?:\**):?/gmi,
            replacement: "",
            reason: "Stack Exchange has an advanced revision history system: 'Edit' or 'Update' is unnecessary"
        },
        voting: {
            expr: /([Dd]own|[Uu]p)[\s*\-]vot/g,
            replacement: "$1vote",
            reason: "the proper spelling (despite the tag name) is '$1vote' (one word)"
        },
        mysite: {
            expr: /mysite\./g,
            replacement: "example.",
            reason: "links to mysite.domain are not allowed: use example.domain instead"
        }

        //expansion reminder: let's support those non web devs with capitalization for popular languages such as C#
    };

    // Populate funcs
    App.popFuncs = function () {
        // This is where the magic happens: this function takes a few pieces of information and applies edits to the post with a couple exceptions
        App.funcs.fixIt = function (input, expression, replacement, reasoning) {

            // Scan the post text using the expression to see if there are any matches
            var match = input.search(expression);

            // If so, increase the number of edits performed (used later for edit summary formation)
            if (match !== -1) {
                App.globals.editCount++;

                // Later, this will store what is removed for the first case
                var phrase;

                // Then, perform the edits using replace()
                // What follows is a series of exceptions, which I will explain below; I perform special actions by overriding replace()

                // This is used for removing things entirely without giving a replacement; it matches the expression and then replaces it with nothing
                if (replacement === "") {
                    input = input.replace(expression, function (data, match1) {

                        // Save what is removed for the edit summary (see below)
                        phrase = match1;

                        // Replace with nothing
                        return "";
                    });

                    // This is an interesting tidbit: if you want to make the edit summaries dynamic, you can keep track of a match that you receive
                    //from overriding the replace() function and then use that in the summary
                    reasoning = reasoning.replace("$1", phrase);

                    // This allows me to combine the upvote and downvote replacement schemes into one
                } else if (replacement == "$1vote") {
                    input = input.replace(expression, function (data, match1) {
                        phrase = match1;
                        return phrase + "vot";
                    });
                    reasoning = reasoning.replace("$1", phrase.toLowerCase());

                    // This is used to capitalize letters; it merely takes what is matched, uppercases it, and replaces what was matched with the uppercased verison
                } else if (replacement === "$1") {
                    input = input.replace(expression, function (data, match1) {
                        return match1.toUpperCase();
                    });

                    // Default: just replace it with the indicated replacement
                } else {
                    input = input.replace(expression, replacement);
                }

                // Return a dictionary with the reasoning for the fix and what is edited (used later to prevent duplicates in the edit summary)
                return {
                    reason: reasoning,
                    fixed: input
                };
            } else {

                // If nothing needs to be fixed, return null
                return null;
            }
        };

        // Omit code
        App.funcs.omitCode = function (str, type) {
            console.log(str);
            str = str.replace(App.globals.checks[type], function (match) {
                App.globals.replacedStrings[type].push(match);
                return App.globals.placeHolders[type];
            });
            return str;
        };

        // Replace code
        App.funcs.replaceCode = function (str, type) {
            for (var i = 0; i < App.globals.replacedStrings[type].length; i++) {
                str = str.replace(App.globals.placeHolders[type], App.globals.replacedStrings[type][i]);
            }
            return str;
        };

        // Eliminate duplicates in array (awesome method I found on SO, check it out!)
        App.funcs.eliminateDuplicates = function (arr) {
            var i,
            len = arr.length,
                out = [],
                obj = {};

            for (i = 0; i < len; i++) {
                obj[arr[i]] = 0;
            }
            for (i in obj) {
                out.push(i);
            }
            return out;
        };

        // Wait for relevant dynamic content to finish loading
        App.funcs.dynamicDelay = function (callback) {
            App.selections.buttonBar = $('#wmd-button-bar-' + App.globals.questionNum);

            // When button bar updates, dynamic DOM is ready for selection
            App.selections.buttonBar.unbind().on('DOMSubtreeModified', function () {

                // Avoid running it more than once
                if (!App.globals.barReady) {
                    App.globals.barReady = true;

                    // Run asynchronously - this lets the bar finish updating before continuing
                    setTimeout(function () {
                        callback();
                    }, 0);
                }
            });
        };

        // Populate or refresh DOM selections
        App.funcs.popSelections = function () {
            App.selections.redoButton = $('#wmd-redo-button-' + App.globals.questionNum);
            App.selections.bodyBox = $(".wmd-input");
            App.selections.titleBox = $(".ask-title-field");
            App.selections.summaryBox = $("#edit-comment");
        };

        // Populate edit item sets from DOM selections - currently does not support inline edits
        App.funcs.popItems = function () {
            App.items[0] = {
                title: App.selections.titleBox.val(),
                body: App.selections.bodyBox.val(),
                summary: ''
            };
        };

        // Insert editing button(s)
        App.funcs.createButton = function () {

            // Insert button
            App.selections.redoButton.after(App.globals.buttonHTML);

            // Insert spacer
            App.selections.redoButton.after(App.globals.spacerHTML);

            // Add new elements to selections
            App.selections.buttonWrapper = $('#ToolkitButtonWrapper');
            App.selections.buttonFix = $('#ToolkitFix');
            App.selections.buttonInfo = $('#ToolkitInfo');
        };

        // Style button
        App.funcs.styleButton = function () {
            App.selections.buttonWrapper.css({
                'position': 'relative',
                    'left': '430px'
            });
            App.selections.buttonFix.css({
                'position': 'static',
                    'float': 'left',
                    'border-width': '0px',
                    'background-color': 'white',
                    'background-image': 'url("http://i.imgur.com/79qYzkQ.png")',
                    'background-size': '100% 100%',
                    'width': '18px',
                    'height': '18px',
                    'outline': 'none'
            });
            App.selections.buttonInfo.css({
                'position': 'static',
                    'float': 'left',
                    'margin-left': '5px',
                    'font-size': '12px',
                    'color': '#424242',
                    'line-height': '19px'
            });

            App.selections.buttonFix.hover(function () {
                App.globals.infoContent = App.selections.buttonInfo.text();
                App.selections.buttonInfo.text('Fix the content!');
                App.selections.buttonFix.css({
                    'background-image': 'url("http://i.imgur.com/d5ZL09o.png")'
                });
            }, function () {
                App.selections.buttonInfo.text(App.globals.infoContent);
                App.selections.buttonFix.css({
                    'background-image': 'url("http://i.imgur.com/79qYzkQ.png")'
                });
            });
        };

        // Listen to button click
        App.funcs.listenButton = function () {
            App.selections.buttonFix.click(function (e) {
                e.preventDefault();
                if (!App.globals.editsMade) {

                    // Refresh item population
                    App.funcs.popItems();

                    // Pipe data through editing modules
                    App.pipe(App.items, App.globals.pipeMods, App.globals.order);
                    App.globals.editsMade = true;
                }
            });
        };

        // Handle pipe output
        App.funcs.output = function (data) {
            App.selections.titleBox.val(data[0].title);
            App.selections.bodyBox.val(data[0].body);
            App.selections.summaryBox.val(data[0].summary);

            // Update the comment: focusing on the input field to remove placeholder text,
            //but scroll back to the user's original location
            var currentPos = document.body.scrollTop;
            $("#wmd-input").focus();
            $("#edit-comment").focus();
            $("#wmd-input").focus();
            window.scrollTo(0, currentPos);
            App.globals.infoContent = App.globals.editCount + ' changes made';
            App.selections.buttonInfo.text(App.globals.editCount + ' changes made');
        };
    };

    // Pipe data through modules in proper order, returning the result
    App.pipe = function (data, mods, order) {
        console.log("Piping edits...");
        var modName;
        for (var i in order) {
            modName = order[i];
            data = mods[modName](data);
            console.log(data[0].body);
        }
        App.funcs.output(data);
        console.log("Edits complete!");
    };

    // Init app
    App.init = function () {
        App.popFuncs();
        App.funcs.dynamicDelay(function () {
            App.funcs.popSelections();
            App.funcs.createButton();
            App.funcs.styleButton();
            App.funcs.popItems();
            App.funcs.listenButton();
        });
    };

    App.globals.pipeMods.omit = function (data) {
        data[0].body = App.funcs.omitCode(data[0].body, "block");
        data[0].body = App.funcs.omitCode(data[0].body, "inline");
        return data;
    };

    App.globals.pipeMods.replace = function(data){
        data[0].body = App.funcs.replaceCode(data[0].body, "block");
        data[0].body = App.funcs.replaceCode(data[0].body, "inline");
        return data;
    };

    App.globals.pipeMods.edit = function (data) {

        // Visually confirm edit - SE makes it easy because the jQuery color animation plugin seems to be there by default
        App.selections.bodyBox.animate({
            backgroundColor: '#c8ffa7'
        }, 10);
        App.selections.bodyBox.animate({
            backgroundColor: '#fff'
        }, 1000);

        //loop through all editing rules
        for (var j in App.edits) {
            if (App.edits.hasOwnProperty(j)) {

                // Check body
                var fix = App.funcs.fixIt(data[0].body, App.edits[j].expr, App.edits[j].replacement, App.edits[j].reason);
                if (fix) {
                    App.globals.reasons[App.globals.numReasons] = fix.reason;
                    data[0].body = fix.fixed;
                    App.globals.numReasons++;
                    App.edits[j].fixed = true;
                }

                // Check title
                fix = App.funcs.fixIt(data[0].title, App.edits[j].expr, App.edits[j].replacement, App.edits[j].reason);
                if (fix) {
                    data[0].title = fix.fixed;
                    if (!App.edits[j].fixed) {
                        App.globals.reasons[App.globals.numReasons] = fix.reason;
                        App.globals.numReasons++;
                        App.edits[j].fixed = true;
                    }
                }
            }
        }

        //eliminate duplicate reasons
        App.globals.reasons = App.funcs.eliminateDuplicates(App.globals.reasons);

        for (var z = 0; z < App.globals.reasons.length; z++) {

            //check that summary is not getting too long
            if (data[0].summary.length < 200) {

                //capitalize first letter
                if (z === 0) {
                    data[0].summary += App.globals.reasons[z][0].toUpperCase() + App.globals.reasons[z].substring(1);

                    //post rest of reasons normally
                } else {
                    data[0].summary += App.globals.reasons[z];
                }

                //if it's not the last reason
                if (z !== App.globals.reasons.length - 1) {
                    data[0].summary += "; ";

                    //if at end, punctuate
                } else {
                    data[0].summary += ".";
                }
            }
        }

        return data;
    };

    // Start
    App.init();

};

// Inject the main script
var script = document.createElement('script');
script.type = "text/javascript";
script.textContent = '(' + main.toString() + ')();';
document.body.appendChild(script);

If any of you are into that sort of thing, feel free to make a pull request sometime.

\$\endgroup\$
  • \$\begingroup\$ Are you sure you want to iterate over an object with for..in ? stackoverflow.com/questions/500504/… \$\endgroup\$ – Stop ongoing harm to Monica Oct 31 '14 at 7:47
  • 2
    \$\begingroup\$ @janos not after reading that :3 \$\endgroup\$ – Viziionary Oct 31 '14 at 7:57
  • \$\begingroup\$ By the way if anyone is wondering why the edit items are stored as sets (objects) in an array, it's because privledged users have capability to edit in-line where there would be lots of items to edit separately (question + all answers). While this functionality isn't supported yet, what you're seeing is me optimizing for its future implementation. \$\endgroup\$ – Viziionary Oct 31 '14 at 21:15
  • \$\begingroup\$ Ouch! Premature optimization... ;) \$\endgroup\$ – GeroldBroser reinstates Monica Apr 15 '17 at 18:07
5
\$\begingroup\$

Your mod collection is not declared in a DRY manner, this

//define modules in one place
App.pipeMods = {
    edit: function (data) {
        return (data + " Edited!");
    },
    omitCode: function (data) {
        return (data + " Code omitted!")
    },
    checkSpelling: function (data) {
        return (data + " Spelling Checked!")
    }
}

//define order in which mods affect data
App.order = [
    "omitCode",
    "edit",
    "checkSpelling"];

has the function names twice. I would simply declare pipeMods as an array which has an inherent order.

App.modules = [
    function edit (data) {
        return (data + " Edited!");
    },
    function omitCode (data) {
        return (data + " Code omitted!")
    },
    function checkSpelling (data) {
        return (data + " Spelling Checked!")
    }
];

I would consider defining the actual code outside of the array declaration.

Then

//Voila
App.pipe = function (data, modules) {

    modules.forEach( function( module ){
        data = module(data);    
    });
    return data;
}

For the rest of the code, from a quick once over:

  • App.globals.numReasons = 0; seems suspicious, you should not need it. Perhaps look up how Array.push works ?
  • This part is very painful:

    for (var z = 0; z < App.globals.reasons.length; z++) {
    
          //check that summary is not getting too long
          if (data[0].summary.length < 200) {
    
              //capitalize first letter
              if (z === 0) {
                  data[0].summary += App.globals.reasons[z][0].toUpperCase() + App.globals.reasons[z].substring(1);
    
                  //post rest of reasons normally
              } else {
                  data[0].summary += App.globals.reasons[z];
              }
    
              //if it's not the last reason
              if (z !== App.globals.reasons.length - 1) {
                  data[0].summary += "; ";
    
                  //if at end, punctuate
              } else {
                  data[0].summary += ".";
              }
          }
      }
    

    The constant repetition of an overly long App.globals.reasons and data[0].summary and a lack of ternary operator usage make this too hard to read. I would go for

    data[0].summary = generateSummary( App.globals.reasons );
    
    function generateSummary( reasons ){
      var summary = reasons.join('; ');
      summary = summary.charAt(0).toUpperCase() + summary.slice(1) + '.';
      return summary.slice( 0, 200 );
    }
    
\$\endgroup\$

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.