4
\$\begingroup\$

I'm trying to get an array of all "form" objects within a large "masterFormObject" object. Is this the cleanest way to do so, or is there a better way I can check if the form objects themselves are not undefined directly and check both the applicant and account portion of this form object without having two separate blocks of undefined checks / forEach loops?

vm.getAllForms = function (type) {

  var allForms = [];
  var accountForms = [];
  var applicantForms = [];

  if (undefined != vm.masterFormObject) {
    if (undefined != vm.masterFormObject.payload) {
      if (undefined != vm.masterFormObject.payload.accounts) {
        vm.masterFormObject.payload.accounts.forEach(function (account) {
          if (undefined != account.Forms) {

            account.Forms.forEach(function (Form) {

              allForms.push(Form);
              accountForms.push(Form);

            });
          }
        });
      }

      if (undefined != vm.masterFormObject.payload.accounts) {
        vm.masterFormObject.payload.applicants.forEach(function (applicant) {
          if (undefined != applicant.Forms) {
            applicant.Forms.forEach(function (Form) {
              allForms.push(Form);
              applicantForms.push(Form);
            });
          }
        });
      }
    }
  }
\$\endgroup\$
1
  • \$\begingroup\$ Welcome to code review. I hope you get some good answers. \$\endgroup\$
    – pacmaninbw
    Commented Aug 30, 2016 at 12:39

2 Answers 2

1
\$\begingroup\$

Your nested "ifs" make the code very hard to read. I would think about changing your approach to something like

if (undefined === vm.masterFormObject) {
    console.log('Some error message.');
    return;
}
if (undefined === vm.masterFormObject.payload) {
    console.log('Some other message');
    return;
}
// and so on

// or if inside a loop
if (undefined === account.Forms) {
    console.log('Some error message.');
    // continue to next iteration
    continue;
}
\$\endgroup\$
0
\$\begingroup\$

Interesting question,

I think you wanted to check for vm.masterFormObject.payload.applicants not being undefined right before you do a forEach on it?

I would chain the undefined checks and use a falsey/truthy approach:

  if ( vm.masterFormObject && 
       vm.masterFormObject.payload && 
       vm.masterFormObject.payload.accounts) {
    //Do the thing
  }

Furthermore, since both accounts and applicants have data in .forms I would use a function to extract that.

Also, you have written your own version basically of Array.concat(), I would just use concat to keep it simple.

All in all that gives something like

vm.getAllForms = function (type) {

  var allForms = [],
      accountForms = [];
      applicantForms = [];

  function extractForms( records ){
    var forms = [];
    records.forEach(function (record) {
      if( record.Forms ){
        forms = forms.concat( record.Forms ); 
      }
    }
    return forms;
  }

  if ( vm.masterFormObject && 
       vm.masterFormObject.payload ){
    var payload = vm.masterFormObject.payload;
    if( payload.accounts ) {
      accountForms = extractForms( payload.accounts );
    }
    if( payload.applicants ) {
      applicantForms = extractForms(payload.applicants );
    }
  }
  allForms = accountForms.concat( applicantForms );

}

If this was a personal project I would have used this for extractForms:

  function extractForms( records ){
    var forms = [];
    records.forEach(function (record) {
        forms = forms.concat( record.Forms || [] ); 
    }
    return forms;
  }

It basically replaces record.Forms with an empty array if the value is undefined, it just looks more elegant to me, but probably would not pass most corporate code reviews.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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