-1
\$\begingroup\$

I've wrote a piece of code today that really make me sick about:

if ( account.plan_id ) {
    const featuresToCheck = [];
    for (const vertical of Object.keys(account.account_settings.enabled_features)) {
        for (const feature of Object.keys(account.account_settings.enabled_features[vertical])){
            featuresToCheck.push(`${vertical}.${feature}`);
        }
    }
    planService.filterFeatures(account.plan_id.id,featuresToCheck).then((filtered) => {
        filtered.forEach((feature) => {
            const [ vert , feat] = feature.split('.');
            out.account_settings.enabled_features[vert][feat].is_active = false;
        });
        customFieldsService.getAccountCustomFields(account_id, out)
        .then(function(){
            return cb();
        })
        .fail(cb);
    });
} else {
    customFieldsService.getAccountCustomFields(account_id, out)
    .then(function(){
        return cb();
    })
    .fail(cb);
}

Which could be best practice to handle cases like this? Unfortunately I cannot transform it in async/await.

\$\endgroup\$
4
  • 2
    \$\begingroup\$ Please heed How do I ask a Good Question? In particular, title your question for what the code is to accomplish. Edit into the question what will it be used for and how. Can you put a finger on what in particular you don't like about the code presented? \$\endgroup\$
    – greybeard
    Mar 28, 2020 at 7:10
  • \$\begingroup\$ Hi, sorry for my mistake. yes, the part i don't like at all is the repetition of the code customFieldsService.getAccountCustomFields . Thanks all for the help and clarification \$\endgroup\$
    – Tiziano
    Mar 30, 2020 at 15:02
  • \$\begingroup\$ Why don't you simply extract it out of the if-else? Right in front of the .fail(cb) on the almost last line? \$\endgroup\$
    – Mast
    Apr 3, 2020 at 5:59
  • \$\begingroup\$ What does this code do? Please add a description so people can easily understand what it is doing. \$\endgroup\$
    – Peilonrayz
    Apr 6, 2020 at 20:28

1 Answer 1

1
\$\begingroup\$

If the part you don't like it the repetition of calling customFieldsServer.getAccountCustomFields(...) in two separate places, then you can avoid that without using async/await like this by create a common promise variable that each branch of the if/else fills in so after the if/else progress can continue using the common promise:

let p;
if ( account.plan_id ) {
    const featuresToCheck = [];
    for (const vertical of Object.keys(account.account_settings.enabled_features)) {
        for (const feature of Object.keys(account.account_settings.enabled_features[vertical])){
            featuresToCheck.push(`${vertical}.${feature}`);
        }
    }
    p = planService.filterFeatures(account.plan_id.id,featuresToCheck).then((filtered) => {
        filtered.forEach((feature) => {
            const [ vert , feat] = feature.split('.');
            out.account_settings.enabled_features[vert][feat].is_active = false;
        });
    });
} else {
    p = Promise.resolve();
}
p.then(() => {
    customFieldsService.getAccountCustomFields(account_id, out).then(function(){
        return cb();
    }).fail(cb);
});

But, this would be simpler if you could use async/await and if your promises were standard-type promises (the .fail() you were using is non-standard):

try {
    if ( account.plan_id ) {
        const featuresToCheck = [];
        for (const vertical of Object.keys(account.account_settings.enabled_features)) {
            for (const feature of Object.keys(account.account_settings.enabled_features[vertical])){
                featuresToCheck.push(`${vertical}.${feature}`);
            }
        }
        let filtered = await planService.filterFeatures(account.plan_id.id,featuresToCheck);
        filtered.forEach((feature) => {
            const [ vert , feat] = feature.split('.');
            out.account_settings.enabled_features[vert][feat].is_active = false;
        });
    }
    await customFieldsService.getAccountCustomFields(account_id, out);
    cb();
} catch(e) {
    cb(e);
}
\$\endgroup\$

Not the answer you're looking for? Browse other questions tagged or ask your own question.