4
\$\begingroup\$

Here's a function I wrote - am I getting DRY about right? I could add another argument to paramFilter maybe, for even less reuse. Or have I gone too overboard as it is?

function goMageHack() {
    var ampFilter = function (str) {
        var amps = ["&", "amp%3B"];
        for (var j = 0; j < amps.length; j++) {
            str = fullReplace(amps[j],"amp;",str);
        }
        return str;
    }

    var paramFilter = function(element, param) {
        var $element = jQuery(element);
        for (var i = 0; i < param.length; i++) {
            if ($element.attr(param[i])) {
                $element.attr(param[i], ampFilter($element.attr(param[i])));
                $element.attr(param[i], fullReplace("amp;", "&", $element.attr(param[i])));
            }
        }
    }

    jQuery(".pages,.sort-by").find("a").each(function (_, element) {
        paramFilter(element, ["data-param", "href"]);
    });

    jQuery(".limiter").find("option").each(function(_, element){
        paramFilter(element, ["value"]);
    });
}

For completeness, since it is called from that function:

function fullReplace(needle, haystack, str) {
    str = String(str);
    var newStr;
    while ((newStr = str.replace(needle, haystack)) !== str) {
        str = newStr;
    }
    return newStr;
}

Addition

function handleChangingDropBox(attrib, that) {
            var $this = jQuery(that);
            var activeSet = Number($this.children(":selected").data("checkboxkey"));
            var checkBoxes = checkBoxSets[activeSet];
            var size;


            if (currentlySelectedDropDownBox !== null) {
                checkBoxSets[currentlySelectedDropDownBox].trigger("click");
            }

            currentlySelectedDropDownBox = activeSet;

            if (size = checkBoxes.size()) {
                var lastCheckBox = checkBoxes.last();
                if (size !== 1) {
                    if ($this.val() === "choose") {
                        var dirtyParams = splitUrlIntoParams(lastCheckBox.data('param'));
                        delete dirtyParams[attrib];
                        lastCheckBox.attr("data-param", createAttribString(dirtyParams));
                        lastCheckBox.trigger("click");
                        currentlySelectedDropDownBox = null;
                    }
                    else {
                        var newParams = [];
                        var not = checkBoxes.not(lastCheckBox);
                        not.each(
                            function (index, element) {
                                var $element = jQuery(element);
                                newParams.push(getTheDiffForATextBox($element, lastCheckBox, attrib));
                            }
                        );
                        createTheDataAttribsForTheLastCheckBox(lastCheckBox, newParams, not.first(), attrib);
                    }
                }
                else {
                    var differentindex = (activeSet == 0) ? 1 : 0;
                    var notCheckBox = checkBoxSets[differentindex].first();
                    createTheDataAttribsForTheLastCheckBox(lastCheckBox, [], notCheckBox, attrib);
                }
                lastCheckBox.trigger("click");


            }
\$\endgroup\$
  • \$\begingroup\$ Could you post a demo at jsfiddle.net to show how it works? It would help find a better way to do this DRYer. \$\endgroup\$ – elclanrs May 30 '14 at 3:48
  • \$\begingroup\$ jsfiddle.net/Lfzfd - I have had to change a bit to get it "working" (in the sense of parsing these attributes" on jsfiddle. I am thinking that maybe I should post more of the source, it's from about 800 lines of js that modify the output of the gomage advanced navigation extension for magento in various ways. (well, modify the output of a modified version of the extension, most stuff is done serverside) \$\endgroup\$ – David Jordan May 30 '14 at 12:37
  • 1
    \$\begingroup\$ Not quite sure what to learn from the jsfiddle: It has a syntax error, and using the dropdowns just reports "GomageNavigation is not defined". A brief explanation like "I have x and I want to do y because z" with a simple input/expected output example would be nice. Right now it's a lot of code without a whole lot of context. I think it's got something to do with replacing ampersands in some element attributes...? \$\endgroup\$ – Flambino Jun 1 '14 at 2:06
2
\$\begingroup\$

I found something in your fullReplace function,

function fullReplace(needle, haystack, str) {
    str = String(str);
    var newStr;
    while ((newStr = str.replace(needle, haystack)) !== str) {
        str = newStr;
    }
    return newStr;
}

You can drop the while statement and write it like this

function fullReplace(needle, haystack, str) {
    str = String(str);
    str = str.replace(/needle/g, haystack)
    return str;
}

This will match every instance in the string because it is a global match. You can also do a case insensitive match if you need to.

Documentation for Replace


From what Flambino said I came up with this

var ampFilter = function (str) {
    return str.replace(/(&amp;|amp%3B)/g, 'amp;');
}

This is where you originally called the fullReplace function. With this you shouldn't need to use the fullReplace function.

\$\endgroup\$
  • \$\begingroup\$ lol this is the second time I think? I did only include the fullReplace function for completeness*, I am really only looking for review of the other bit. But again (like the last time!) this is totally my fault dude so I've given you a vote. I think what I'm going to do is sanitize the entire cleanup javascript, which comes to around 750 lines off the top of my head, and post that because it's much more substantial and has a lot more comments that can be made... actually, would it be alright to send you more info by a private channel for advice on effective anonymisation? \$\endgroup\$ – David Jordan May 31 '14 at 0:02
  • \$\begingroup\$ *original has comment about what you identified although I wrote it too long ago to remember that! \$\endgroup\$ – David Jordan May 31 '14 at 0:04
  • 1
    \$\begingroup\$ Hang on, you're searching str and replacing with haystack? I'd say that should probably be the other way around. Either way, no need for a function, as it's a one-liner haystack.replace(/needle/g, String(str)) \$\endgroup\$ – Flambino Jun 1 '14 at 1:55
  • 1
    \$\begingroup\$ @Malachi That regular expression will search for the string "ampj" (there's no interpolation). I'll take the liberty of fixing it :) \$\endgroup\$ – Flambino Jun 1 '14 at 2:09
  • 1
    \$\begingroup\$ Decided to be a little brutal in my edit. Hope you don't mind. Since I figured the strings to search for are hardcoded anyway, they might as well be hardcoded in the regex rather than in an array and requiring a loop, etc. \$\endgroup\$ – Flambino Jun 1 '14 at 2:14

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.