1
\$\begingroup\$

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);
\$\endgroup\$
  • \$\begingroup\$ what language is this? you have $_GET[], PHP? but you also have variables with no $, JS? \$\endgroup\$ – Joseph May 11 '12 at 16:01
  • \$\begingroup\$ I've added some more code which should explain! it's all JS \$\endgroup\$ – HandiworkNYC.com May 11 '12 at 16:10
  • 1
    \$\begingroup\$ It's considered bad practice to use the $ in variable names in javascript - the symbol was originally intended for machine use. Yes, I know libraries love it, but that's just one more reason not to use it - confusion with library code. \$\endgroup\$ – delete me May 11 '12 at 19:47
  • \$\begingroup\$ also, you don't have to pass $filterTerms - it is present in the scope in which the function was declared, so the function has access to it, even when it is not passed. \$\endgroup\$ – delete me May 11 '12 at 19:51
2
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ Very cool solution. Do you think the one in my updated answer at the bottom is more efficient? I think it's more readable at least... \$\endgroup\$ – HandiworkNYC.com May 11 '12 at 16:15
  • \$\begingroup\$ updated after your update to the question and comment - I doubt there is any different in efficiency, it's simply more flexible to use a loop and process it. \$\endgroup\$ – AD7six May 11 '12 at 16:33
  • \$\begingroup\$ +1 as anything I'd do would probably be an attempt to explain the same. not I'd sure that the appending to the DOM issue really needs to be brought up, though, for such a small set of things. \$\endgroup\$ – JayC May 11 '12 at 16:38
1
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ Jay-- I've updated my answer with more info. What you're describing here is essentially the direction I'd like to go in! If you elaborate a little more that would be great. Thanks! \$\endgroup\$ – HandiworkNYC.com May 11 '12 at 15:57
  • \$\begingroup\$ Also this snippet is the only place in the code where I use those variables (area, style, type, feature) \$\endgroup\$ – HandiworkNYC.com May 11 '12 at 15:59
  • \$\begingroup\$ productionlocations.com is the live site... this function deals with searching / filtering properties FYI \$\endgroup\$ – HandiworkNYC.com May 11 '12 at 15:59
  • \$\begingroup\$ wait... $_GET? That's likely PHP not javascript. \$\endgroup\$ – JayC May 11 '12 at 16:08
  • \$\begingroup\$ I've updated my answer... it's all javascript. I know what language I'm coding :) \$\endgroup\$ – HandiworkNYC.com May 11 '12 at 16:11
1
\$\begingroup\$

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.

\$\endgroup\$
1
\$\begingroup\$

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