Refactoring and condensing a Javascript function with many repeat conditions, strings, and variables

Question

I have this function which is pretty easy to read and works just fine. I'm looking to advance my coding abilities by writing more concise code, though.

Does anyone know of an elegant way to condense this down? Any pointers very much appreciated! Thank you!

var $_GET = {};

document.location.search.replace(/\??(?:([^=]+)=([^&]*)&?)/g, function () {
    function decode(s) {
        return decodeURIComponent(s.split("+").join(" "));
    }

    $_GET[decode(arguments[1])] = decode(arguments[2]);
});

function create_filter_input($name, $termname) {
    var $input = '<input class="query-input" type="hidden" value="' + $termname + '" name="' + $name + '" />';
    return $input;
}

area = $_GET["area"],
type = $_GET["property_type"],
style = $_GET["style"],
feature = $_GET["feature"],

if(area || type || style || feature) {                                      
    if(area) { 
        areainput = create_filter_input('area',area); 
        $filterTerms.append(areainput);
    }

    if(type) { 
        typeinput = create_filter_input('property_type',type);  
        $filterTerms.append(typeinput);
    }

    if(style) {
        styleinput = create_filter_input('style',style);
        $filterTerms.append(styleinput);
    }

    if(feature) { 
        featureinput = create_filter_input('feature',feature); 
        $filterTerms.append(featureinput);
    }
}

---

This is my current solution based on your answers! I think it's pretty concise. Let me know what you think! thanks again.

function create_filter_input($name, $termname, $filterterms) {
    if($termname) {
        var $input = '<input class="query-input" type="hidden" value="' + $termname + '" name="' + $name + '" />';
        $filterterms.append($input);
    }
}

create_filter_input('area',area,$filterTerms);
create_filter_input('property_type',type,$filterTerms);
create_filter_input('style',style,$filterTerms);
create_filter_input('feature',feature,$filterTerms);

Answer 1

Try to avoid repetition.

var args, i, check, len, filtered;

function parseUrl() {
    var getArgs = {};

    function decode(s) { // declare this function only once, don't declare functions inside any kind of loop
        return decodeURIComponent(s.replace(/\+/g, ' '));

    document.location.search.replace(/\??(?:([^=]+)=([^&]*)&?)/g, function () {
        getArgs[decode(arguments[1])] = decode(arguments[2]);
    });

    return getArgs;
}

function create_filter_input($name, $termname) {
    var $input = '<input class="query-input" type="hidden" value="' + $termname + '" name="' + $name + '" />';
    return $input;
}

args = parseUrl();
check = ['area', 'property_type', 'style', 'feature'];
len=check.length;

for(var i=0; i<len; i++) {
    if (!args[check[i]]) {
        continue;
    }
    filtered = create_filter_input(check[i], args[check[i]]); 
    $filterTerms.append(filtered);
}

Try to write code anticipating change and write it to make your life easy. For example this:

type = $_GET["property_type"],

creates a variable named type but you then effectively need a map to get back to "property_type":

create_filter_input('property_type',type);

Is the same as:

create_filter_input('property_type', $_GET["property_type"]);

(Which, incidentally is the same as: create_filter_input('property_type', $_GET.property_type); - use dot notation where possible.)

If you write it like that - seeing that you are passing (almost) the same argument in all use cases to your create_filter_input function - you can start to see/think that you don't need 2 local variables - just having "property_type" is enough.

As one of the other answers says - don't serially append to the dom. each write is relatively expensive. it's better to build a dom fragment and append it to the dom in one go.

Answer 2

The main thing I would question is why you have so many boolean or rather "truthy" values in the first place. If you're using area, type, style, feature more than here, you might be essentially duplicating decision logic all over the place. if the conditions are all exclusive, it'd be better to have

1. a set of objects (or rather Javascript "classes" or "constructors" which we can use to create said objects") with a commonly named method, each doing their respective thing, and
2. a factory method that determines which of those objects to create.

After all that, the whole block of code (bessides the constructor and object factory definitions might become something like

var filterThing = objectFactory.makMeAFilter(x,y,z);
filterThing.addInput($filterterms);

HOWEVER, if the conditions aren't exclusionary.. well, you still have options, and you could do the object factory thing, but it might just be better to leave it alone. It really depends on how the area, type, style, feature things are used elsewhere.

Answer 3

Expanding on @Joseph's answer - and I haven't spent much time with javascript, so treat with caution - I would think you could write a method to simplify it further.

var container = $('<div>'); //create container that is not yet in DOM

process(container, area, 'area')
process(container, type, 'property_type')
process(container, style, 'style')
process(container, feature, 'feature')

container.contents().appendTo($filterTerms); //append to DOM

...

function process($container, $category, $tag) {
    if ($category) {
        $container.append(create_filter_input($tag, $category));
    }
}

This kind of Extract Method refactoring is good low-hanging fruit with duplication of this sort.

Answer 4

What's the idea of having those OR checking, when you check them again individually?

if(area || type || style || feature){...

If none hold true, they never run anyway. If one exists, all of your 4 ifs run also, and checking again

Assuming that they are not DOM elements, and the variables are not used anywhere else, you can do this:

if (area) {
    $filterTerms.append(create_filter_input('area', area));
}

if (type) {
    $filterTerms.append(create_filter_input('property_type', type));
}

if (style) {
    $filterTerms.append(create_filter_input('style', style));
}

if (feature) {
    $filterTerms.append(create_filter_input('feature', feature));
}

but if they are DOM elements (and it looks like jQuery):

var container = $('<div>'); //create container that is not yet in DOM

if (area) {
    container.append(create_filter_input('area', area));
}

if (type) {
    container.append(create_filter_input('property_type', type));
}

if (style) {
    container.append(create_filter_input('style', style));
}

if (feature) {
    container.append(create_filter_input('feature', feature));
}

container.contents().appendTo($filterTerms); //append to DOM in one go