2
\$\begingroup\$

Here is a simple filter function I have:

function filter (attributes, data, include) {
    if (include) {
        let res = {}
        if (typeof attributes === 'string') {
            res[attributes] = data[attributes]
            return res
        }
        if (attributes.length) {
            attributes.forEach(attr => res[attr] = data[attr])
        }
        return res
    } else {
        if (typeof attributes === 'string') {
            delete data[attributes]
            return data
        }
        if (attributes.length) {
            attributes.forEach(attr => delete data[attr])
        }
        return data
    }
}

As you can see, these parts are same:

if (typeof attributes === 'string') {
    // some code
    return // some variable
}
if (attributes.length) {
    attributes.forEach(attr => /* some code */)
}
return // some variable

What is the best way to remove this duplicated code?

\$\endgroup\$
2
  • 3
    \$\begingroup\$ The code is not similar, they're different. One is updating property value and other is deleting a property. \$\endgroup\$
    – Tushar
    Sep 7 '16 at 10:18
  • \$\begingroup\$ What I don't understand is on include == true you clone the data object with the results you want back, on include == false you modify the data object instead. It is a weird behavior. \$\endgroup\$ Sep 7 '16 at 11:52
3
\$\begingroup\$

To @Tushar's point, those 2 code blocks are not similar. In fact, I would advise you to turn that function in to 2 functions with a better name. Especially since your function optionally depending on include modifies data which is not a good design. It should either always generate a new object, or always modify data. Probably it should always generate a new object.

Furthemore for greater flexibility, I would not check for attributes.length but for attributes.forEach, this way you know for sure the function/method is defined.

In Lodash the first function would be called pick, and the second function would be omit.

Thinking about this, if you are always going to build a new object, you could remove repetitive coding:

function filter(attributes, data, include) {
    'use strict';

    let keys = Object.keys(data),
        res = {};
    //Turn attributes into an array if needed
    attributes = (typeof attributes === 'string') ? [attributes] : attributes;
    keys.forEach(key => {
        //If its an attribute we include, get it
        //If its an attribute we dont exclude, get it
        if ((include && attributes.indexOf(key) != -1) ||
            (!include && attributes.indexOf(key) == -1)) {
            res[key] = data[key];
        }
    });
    return res;
}
\$\endgroup\$
4
  • \$\begingroup\$ The condition might be more readable as include ^ (attributes.indexOf(key) === -1) \$\endgroup\$ Sep 7 '16 at 15:06
  • \$\begingroup\$ I dont even know what that caret would do? \$\endgroup\$
    – konijn
    Sep 7 '16 at 15:43
  • \$\begingroup\$ XOR. Although I didn't realise that in JS it's bitwise, so the type coercion could cause unwanted behaviour. \$\endgroup\$ Sep 7 '16 at 16:00
  • \$\begingroup\$ I think from a CodeReview perspective ^ is definitely out. I get flack from this community for using bitwise NOT '~' even if it is a match made in heaven to use on the results of indexOf() \$\endgroup\$
    – konijn
    Sep 7 '16 at 16:58

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.