2
\$\begingroup\$

Please go through below code and improve the quality by reducing the IF conditions:

I have 2 jsons, first one is the source and second one is the updated json. I have to compare both values and set true/false flags. Based on the flags i need to proceed further.

I am checking below conditions:

  1. NULL changed to DECLINED
  2. NULL changed to a VALUE
  3. VALUE changed to DECLINED
  4. VALUE1 changed to VALUE2

O/P - canProceed = (nullDec) ? (nullDec && (nullVal || valDec || valVal)) :(nullVal || valDec || valVal);

Plnkr - https://plnkr.co/edit/gKJXibTFQhInBOhz?open=lib%2Fscript.js

import angular from 'angular';

angular.module('plunker', []).controller('MainCtrl', function($scope) {
  $scope.name = 'Plunker';
  
    var json1={id:"01", name:"Jon", city:null, state:"NY", phone:null, zip:"06033"};
    var json2={id:"02", name:"Kris", city:"DECLINED", state:"DECLINED", phone:"860-000-0000", zip:"06033"};
    var json3={id:"02", name:"Sam", city:"EDISON", state:"DECLINED", phone:"860-000-0000", zip:"06033"};

    compareJsons(json1, json2);
    
    function compareJsons(model1, model2){
        console.log(model1);
        console.log(model2);
        
        var decline = "DECLINED";
        var canProceed = false;
        var nullVal = false;
        var nullDec = false;
        var valDec = false;
        var valVal = false;
        
        angular.forEach(model1, function(m1Val, m1Key){
           if(!m1Val){
               console.log("Found NULL - "+ m1Key +" - " + m1Val);
               
              angular.forEach(model2, function(m2Val, m2Key){
                  if((m2Key === m1Key) && (m2Val !== m1Val)){
                      console.log(m1Key +" - " + m1Val + "  CHANGED TO  " + m2Key +" - " + m2Val);
                      if((m2Val === decline) && !(m1Val)){
                          nullDec = true;
                      }
                      if(((m2Val) && (m2Val !== decline)) && !(m1Val)){
                          nullVal = true;
                      }
                  } 
              })
           } else {
              angular.forEach(model2, function(m2Val, m2Key){
                  if(m2Key === m1Key && m2Val !== m1Val){
                      if(m2Val === decline){
                            console.log(m1Key +" - " + m1Val + "  CHANGED TO " + m2Key +" - " + m2Val);
                            if(((m1Val) && (m1Val !== decline)) && (m2Val)){
                                valDec = true;
                            }   
                      }else{
                            console.log(m1Key +" - " + m1Val + "  CHANGED TO " + m2Key +" - " + m2Val);
                            if((m1Val) && (m2Val)){
                                valVal = true;
                            }   
                      }
                  }
              });
           }
        });
        
        console.log("NULL to DECLINE    - " + nullDec);
        console.log("NULL to VALUE      - " + nullVal);
        console.log("VALUE to DECLINE   - " + valDec);
        console.log("VALUE1 to VALUE2   - " + valVal);
        
        canProceed = (nullDec) ? (nullDec && (nullVal || valDec || valVal)):(nullVal || valDec || valVal);
        
        console.log("Can Proceed        - " + canProceed);
        
    }
    
});

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

Your code look good, but there is some room for improvements.

Comment your code

Its is a good pratice to comment the behavior that you code should have. There is a good article written by Jeff Atwood called "Code Tells You How, Comments Tell You Why", when you have time you should read it, but the best way that i find to write comments its to write them before the code itself.

So if i had to write your code myself my first step would be something like:

// iterate all properties
    // check if the the first value is defined 
        // ...
    // if the value is not defined then 
        // ...

The hard part its (almost) never writing the code itself but thinking about it, if you have no problems writing these comments you should have no problems writing the code itself. This helps you to make sure that you undertand the problem that you are trying to solve with code, if If you find diffucults in this step you should spend more time thinking about the problem.

Iterators

You are using forEach but seems that you only need to iterate until the first positive result so you may use some, so your function will not need to iterate all the elements, so your function would be something like:

angular.some(model2, function(m2Val, m2Key){
  if((m2Key === m1Key) && (m2Val !== m1Val)){
      console.log(m1Key +" - " + m1Val + "  CHANGED TO  " + m2Key +" - " + m2Val);
      if((m2Val === decline) && !(m1Val)){
          nullDec = true;
      }
      if(((m2Val) && (m2Val !== decline)) && !(m1Val)){
          nullVal = true;
      }
  } 
  return nullDec && nullVal;
})

Conditionals

Inside the if(!m1Val) you have the conditionals (m2Val === decline) && !(m1Val) and (m2Val) && (m2Val !== decline)) && !(m1Val), you don't need to check m1Val again because its was already checked, the same its true to the block of code of your else statment.

Complexity

If you are sure that all the keys in model1 is always exist in model2 you don't need to iterate model2 to get its value.

Instead of the second forEach if the (m2Key === m1Key) you can get the value of the m1Key in the model2 object without the second loop;

This code have O(n^2) complexity:

angular.forEach(model1, function(m1Val, m1Key){
   if(!m1Val){                             
      angular.forEach(model2, function(m2Val, m2Key){
          if((m2Key === m1Key) && (m2Val !== m1Val)){

          } 
      })
   }
}

This one below have O(n), but will need to make sure that all model1 and model2 have the same keys, or add some conditionals to make sure that the behavior is correct.

angular.forEach(model1, function(m1Val, m1Key){
    if (!m1Val) {
        if (m1Val !== model2[m1Key]) {
            // ...
        }
    }           
}

Even without these improvements your code passed all the test cases that i could come up with, so you just need to adjust your style and get deep understanding of the JS api. I can see that you tried to cover all possible cases that is pretty good, you should continue aim for this as much as possible.

\$\endgroup\$
  • 1
    \$\begingroup\$ Hi appreciate your feedback.. i have modified code like below plnkr.co/edit/mdttacWm4I1xDHXM?open=lib%2Fscript.js \$\endgroup\$ – asder May 3 '18 at 3:32
  • \$\begingroup\$ Inside your loop, m2Val is always equals to model2[m1key], besides that the code should be good to go. \$\endgroup\$ – Josenberg May 3 '18 at 17:13

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.