12
\$\begingroup\$

I've written a userscript that pops up a dialog before you take a moderation action (closing, deleting) with some pro-forma comments, so that you can add one of those before you cast your vote.

I'm pretty happy with how I wrote it, but what's clear to me is unlikely to be so to others. Is there anything in this that I could have "phrased" better, or that could be optimized more? Comments on style (or, for that matter, anything else) are also welcome. It also occurs to me that this isn't as extensible as it could be: it's not easy to add new comments. Is there anything I could do to facilitate this?

I've also put this code in a gist, if you want to see it there.

$(document).ready(function() {

    // This section is only not in "Program", below, because fail-fast.

    if(!StackExchange) {
        console.error("SE.MAAC can't run: no StackExchange global");
        return;
    }

    // Utility functions: 'library'-type methods for helping those that perform the major tasks.

    function loadDialog(html_or_element, type) {
        if(type === "html") {
            $("body").loadPopup({
                "lightbox": false,
                "html": html_or_element,
                "target": $("body")
            });
        }
        else if(type === "element") {
            if($(html_or_element).length == 1) {
                $("body").loadPopup({
                    "lightbox": false,
                    "html": $("html_or_element").html(),
                    "target": $("body")
                });
            }
            else {
                throw new ValueError("loadDialog: param 'html_or_element': element does not exist in document tree, or there is more than one matching element.");
            }
        }
        else {
            throw new ValueError("loadDialog: param 'type': Value must be one of 'html' or 'element'.");
        }
    }

    function generateDialogContent(title, commentList) {
        var contentString = '<div class="autocomment-dialog popup"><div><h2 style="margin-bottom:12px;">' + title + '</h2>'
                          + '<ul class="action-list">';
        var keys = Object.keys(commentList);
        for(var i = 0; i < keys.length; i++) {
            var commentTitle = keys[i];
            var commentObject = commentList[commentTitle];
            var text = commentObject["text"];
            var value = commentObject["value"];
            contentString += '<li><label><input type="radio" name="autocomment" value="' + value + '" />';
            contentString += '<span class="action-name">   ' + commentTitle + '</span>';
            contentString += '<span class="action-desc">' + text + '</span></label></li>';
        }
        contentString += '</ul>';
        contentString += '<div class="popup-actions"><div style="float:right;">';
        contentString += '<button class="autocomment-submit">Submit</button></div>';
        return contentString;
    }

    function closeDialogs(selector) {
        $(selector).fadeOut(350, function() {
            $(this).remove();
        });
    }

    function setupContentAndHandlers(data) {
        var keys = Object.keys(data);
        for(var i = 0; i < keys.length; i++) {
            // I'd just access the dataSet by data[i] if I could, but... well, I can't. Have to have the key instead.
            var key = keys[i];
            var dataSet = data[key];

            if(dataSet["comments"] && dataSet["dialogTitle"]) {
                dataSet["content"] = generateDialogContent(dataSet["dialogTitle"], dataSet["comments"]);
            }
            else {
                /* Actually throwing an error here would cause more trouble than the semantic improvement is worth *
                 * because it would halt data processing for every data set. Instead, just forget this data set as *
                 * invalid and move on.                                                                            */
                continue;
            }

            if(dataSet["trigger"] && dataSet["action"] && dataSet["content"]) {
                /* Event handlers here are run via $.proxy with a dataSet parameter because if they aren't then    *
                 * we get the wrong dataSet in use. For example, if the data has two top-level objects for 'close' *
                 * and 'delete', then both handlers use the 'delete' data because that's the dataSet when the      *
                 * event is actually called. By using $.proxy, we avoid this by passing the correct dataSet as an  *
                 * argument to the handler function.                                                               */
                $(document).delegate(dataSet["trigger"], "click", $.proxy(function(dataSet, e) {
                    e.preventDefault();
                    var postElement = $(e.target).closest(".answer, .question");
                    var postType = postElement.hasClass("question") ? "question" : "answer";
                    var id = postElement.data((postType == "question" ? "question" : "answer") + "id");
                    loadDialog(dataSet["content"], "html");
                    $(".autocomment-submit").on("click", function() {
                        dataSet["action"](id);
                    });
                }, null, dataSet));
            }
            else {
                // See notes about continuing in the last else block.
                continue;
            }
        }
    }

    // Action functions: methods that do something (process data, create comments/content, etc).

    function addComment(postId, commentText, callback) {
        $.ajax({
            type: 'POST',
            url: '/posts/' + postId + '/comments',
            data: {
                'comment': commentText,
                'fkey': StackExchange.options.user.fkey
            },
            success: function(data) {
                console.log("addComment done");
                console.log(typeof(callback));
                if(typeof(callback) === "function") {
                    callback(true);
                }
            },
            fail: function(jqXHR, textStatus, errorThrown) {
                if(typeof(callback) === "function") {
                    callback(false, jqXHR.responseText);
                }
            }
        });
    }

    function handleDialogResult(postId) {
        var comment = $("input:radio[name='autocomment']:checked").siblings(".action-desc").text();
        if(comment) {
            addComment(postId, comment, function(success, errorMessage) {
                console.log(success, errorMessage);
                if(!success) {
                    alert("Failed to comment: " + errorMessage);
                }
                else {
                    closeDialogs(".autocomment-dialog");
                }
            });
        }
        else {
            alert("Select a comment to post.");
        }
    }

    // Data - extra data needed to provide details for methods to work.

    /* Ooooh boy this is a data structure and a half.                                                                *
     *                                                                                                               *
     * Yeah, now get over that feeling, it's not that hard. This is one object, containing all the data you will     *
     * ever need to construct, call, and handle the dialogs. It works like this:                                     *
     *                                                                                                               *
     * You have top-level objects, which are dialog definitions - each top-level object within the data structure    *
     * represents a dialog that is loaded for a specific action.                                                     *
     *                                                                                                               *
     * You have several informational properties within that object. In no particular order: 'trigger' is the        *
     * jQuery element selector that matches elements for which the dialog will be displayed when they are clicked.   *
     * 'action' is the method that does whatever needs to be done with the dialog's result - gets the selected value *
     * and posts it, usually. Will be passed the post ID. 'dialogTitle' is what will be displayed at the top of the  *
     * dialog, in a <h2>, as a title. Finally, you have 'comments', which is another object of objects.              *
     *                                                                                                               *
     * In the 'comments' object, there should be a set of objects, each representing one comment, and at the same    *
     * time, one item in the resulting dialog. They should be of this format:                                        *
     *                                                                                                               *
     *     'comment title': {                                                                                        *
     *         'value': "a value to be inserted as the HTML <input> element's value attribute",                      *
     *         'text': "a long string, containing the MARKDOWN that represents the comment to be posted"             *
     *     }                                                                                                         *
     *                                                                                                               *
     * Enough documentation for you? It's certainly enough for me.                                                   */
    var dialogData = {
        'close': {
            'trigger': 'a.close-question-link',
            'action': handleDialogResult,
            'dialogTitle': 'Add a closure auto-comment',
            'comments': {
                'technical support': {
                    'value': 'tech-support',
                    'text': "Hi there! I'm closing this as off-topic for the reasons mentioned just below your post in the close "
                          + "reason. This site can't help you with technical support for *existing* hardware, only "
                          + "recommendations for new hardware. If you can rephrase your question to ask for a recommendation, "
                          + "we may be able to reopen it. Thanks!"
                },
                'general advice': {
                    'value': 'gen-advice',
                    'text': "Hi there! I've put this question on hold because as it's currently worded, it seems like it's "
                          + "asking for general advice about hardware, which isn't in our scope. If you can reword this to ask "
                          + "instead for a recommendation of a specific piece of hardware you need, we may be able to reopen "
                          + "it. Thanks!"
                }
            }
        },
        'delete': {
            'trigger': 'a[data-delete-prompt]',
            'action': handleDialogResult,
            'dialogTitle': 'Add a deletion auto-comment',
            'comments': {
                'no synopsis': {
                    'value': 'no-syn',
                    'text': "Hi there! I've deleted this answer because it doesn't quite meet our [quality standards]"
                          + "(http://meta.hardwarerecs.stackexchange.com/q/59/8). Please take a moment to read them. "
                          + "Specifically, this answer is lacking a synopsis of the hardware and a description of how it meets "
                          + "the asker's needs - please post a new answer, including those things. Thanks!"
                },
                'advice/no recommendation': {
                    'value': 'advice-no-rec',
                    'text': "It's great that you've got this advice for us, but please remember that [answers are expected "
                          + "to recommend specific hardware](http://meta.hardwarerecs.stackexchange.com/q/59/8). I've deleted "
                          + "this; please have a read of those guidelines, and post a new answer if you've got a recommendation "
                          + "for us - or comment if you want to give advice. Thanks!"
                }
            },
        }
    };

    // "Program" - the bit that actually runs the script.

    setupContentAndHandlers(dialogData);

});
\$\endgroup\$
  • \$\begingroup\$ Wait, is that the autocomment script? \$\endgroup\$ – Mathieu Guindon Nov 30 '15 at 19:42
  • \$\begingroup\$ @Mat'sMug if by the autocomment script you mean the popular one on Stack Apps, no. \$\endgroup\$ – ArtOfCode Nov 30 '15 at 19:43
  • \$\begingroup\$ Oh well, nice post anyway! I hope you get great reviews! \$\endgroup\$ – Mathieu Guindon Nov 30 '15 at 19:47
  • \$\begingroup\$ I hope you remember what happened to the last mod that went through a significant number of posts with the same problem and "fixed" them with a canned comment... :) \$\endgroup\$ – Vogel612 Nov 30 '15 at 19:50
  • 2
    \$\begingroup\$ @Vogel612 I... don't think I was around for that. But still, these are still intended to be used with the same degree of care that any mod action is. \$\endgroup\$ – ArtOfCode Nov 30 '15 at 19:52
3
\$\begingroup\$

Early returns

The top function is the only place you have used an early return. You could do the same to make loadDialog and handleDialogResult less hairy.

Manual HTML construction

It will be hard to maintain afterward. Have you considered a library like Mustache for this purpose?

Everything else

This is an obvious code duplication:

if(type === "html") {
    $("body").loadPopup({
        "lightbox": false,
        "html": html_or_element,
        "target": $("body")
    });
}
else if(type === "element") {
    if($(html_or_element).length == 1) {
        $("body").loadPopup({
            "lightbox": false,
            "html": $("html_or_element").html(),
            "target": $("body")
        });
    }
    // ...
}

Could be rewritten as:

var html = type === "html"
    ? html_or_element
    : $("html_or_element").html();

$("body").loadPopup({
    "lightbox": false,
    "html": html,
    "target": $("body")
});

I would call console.warn here:

else {
    /* Actually throwing an error here would cause more trouble than the semantic improvement is worth *
     * because it would halt data processing for every data set. Instead, just forget this data set as *
     * invalid and move on.                                                                            */
    continue;
}

This code doesn't do anything:

else {
    // See notes about continuing in the last else block.
    continue;
}

addComment function can be shorter if you return a promise:

function addComment(postId, commentText) {
    return $.ajax({
        type: 'POST',
        url: '/posts/' + postId + '/comments',
        data: {
            'comment': commentText,
            'fkey': StackExchange.options.user.fkey
        }
    });
}
addComment(postId, comment)
    .done(function () {
        closeDialogs(".autocomment-dialog");
    })
    .fail(function (jqXHR) {
        alert("Failed to comment: " + jqXHR.responseText);
    })
    .always(function (_, textStatus) {
        console.log(textStatus, jqXHR.responseText);
    });
\$\endgroup\$
1
\$\begingroup\$
 * Enough documentation for you? It's certainly enough for me.

You said it, your documentation is a little over the top, consider only saying/including what's necessary and trim the fat a little, so to speak.

Pavlo's review covered the major things, so I'll review a few minor things.


dialogData:

JSON files can get very large at times, so it's best to keep this in an external file in the same directory, and use the magic of the UserScript @resource header:

// @resource    data      https://linkto.mydata/data.json
...

var dialogData = GM_getResourceText('data');

contentString:

Instead of consistent string concatenation, consider using an array join, like the following:

contentString = ['<li><label><input type="radio" name="autocomment" value="', value, '" />'
       ,'<span class="action-name">   ', commentTitle, '</span>'
       ,'<span class="action-desc">', text, '</span></label></li>'].join(" ");

Array joins are faster operations than string concatenation, but some people prefer the latter, while personal preference, I would suggest considering it.

Additionally, the following could be improved.

    contentString += '<button class="autocomment-submit">Submit</button></div>';
    return contentString;

You could change this to return contentString += '...'; instead.

\$\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.