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: "1ドルI2ドル",
 reason: "basic capitalization"
 },
 so: {
 expr: /(^|\s)[Ss]tack\s*overflow|StackOverflow(.|$)/gm,
 replacement: "1ドルStack Overflow2ドル",
 reason: "'Stack Overflow' is the proper capitalization"
 },
 se: {
 expr: /(^|\s)[Ss]tack\s*exchange|StackExchange(.|$)/gm,
 replacement: "1ドルStack Exchange2ドル",
 reason: "'Stack Exchange' is the proper capitalization"
 },
 expansionSO: {
 expr: /(^|\s)SO(\s|,|\.|!|\?|;|\/|\)|$)/gm,
 replacement: "1ドルStack Overflow2ドル",
 reason: "'SO' expansion"
 },
 expansionSE: {
 expr: /(^|\s)SE(\s|,|\.|!|\?|;|\/|\)|$)/gm,
 replacement: "1ドルStack Exchange2ドル",
 reason: "'SE' expansion"
 },
 javascript: {
 expr: /(^|\s)[Jj]ava\s*script(.|$)/gm,
 replacement: "1ドルJavaScript2ドル",
 reason: "'JavaScript' is the proper capitalization"
 },
 jsfiddle: {
 expr: /(^|\s)[Jj][Ss][Ff]iddle(.|$)/gm,
 replacement: "1ドルJSFiddle2ドル",
 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: "1ドルjQuery2ドル",
 reason: "'jQuery' is the proper capitalization"
 },
 html: {
 expr: /(^|\s)[Hh]tml(?:5*)(\s|$)/gm,
 replacement: "1ドルHTML2ドル",
 reason: "HTML: HyperText Markup Language"
 },
 css: {
 expr: /(^|\s)[Cc]ss(\s|$)/gm,
 replacement: "1ドルCSS2ドル",
 reason: "CSS: Cascading Style Sheets"
 },
 json: {
 expr: /(^|\s)[Jj]son(\s|$)/gm,
 replacement: "1ドルJSON2ドル",
 reason: "JSON: JavaScript Object Notation"
 },
 ajax: {
 expr: /(^|\s)[Aa]jax(\s|$)/gm,
 replacement: "1ドルAJAX2ドル",
 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: "1ドルPHP2ドル",
 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: "1ドルvote",
 reason: "the proper spelling (despite the tag name) is '1ドルvote' (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 == "1ドルvote") {
 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.

Jamal
35.2k13 gold badges134 silver badges238 bronze badges
asked Oct 31, 2014 at 6:57
\$\endgroup\$
4
  • \$\begingroup\$ Are you sure you want to iterate over an object with for..in ? stackoverflow.com/questions/500504/… \$\endgroup\$ Commented Oct 31, 2014 at 7:47
  • 2
    \$\begingroup\$ @janos not after reading that :3 \$\endgroup\$ Commented Oct 31, 2014 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\$ Commented Oct 31, 2014 at 21:15
  • \$\begingroup\$ Ouch! Premature optimization... ;) \$\endgroup\$ Commented Apr 15, 2017 at 18:07

1 Answer 1

4
\$\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 );
    }
    
answered Nov 5, 2014 at 14:07
\$\endgroup\$
0

Your Answer

Draft saved
Draft discarded

Sign up or log in

Sign up using Google
Sign up using Email and Password

Post as a guest

Required, but never shown

Post as a guest

Required, but never shown

By clicking "Post Your Answer", you agree to our terms of service and acknowledge you have read our privacy policy.