Function that checks two objects with same fields and assigns old non-null values to new object if new values are empty or null

Question

I have a function that checks two reference objects. They both have the same properties.

The purpose of this method is to ensure that any non-null old values are not overwritten with null or empty values after an API call. I'd appreciate any feedback.

evaluateEmptyValues: function(reference, originalReference) {
 var vm = this;
 // Get length
 referenceLength = Object.entries(reference).length;
 originalReferenceLength = Object.entries(originalReference).length;
 // Evaluate both if they are the same length -- they always should be
 if (referenceLength == originalReferenceLength) {
 try {
 for (var prop in reference) {
 // First check for undefined or null
 if (reference[prop] != undefined || reference[prop] != null) {
 if (typeof (reference[prop]) == 'string' && reference[prop].trim() == '') {
 // Assign original value to new object if new value is empty string
 reference[prop] = originalReference[prop];
 }
 // Check if current prop in both objects is an object
 if (typeof(reference[prop]) == 'object' && typeof(originalReference[prop]) == 'object') {
 var length = Object.keys(reference[prop]).length;
 // Do another loop
 for (var property in reference[prop]) {
 // Check for undefined or null value in original
 if (originalReference[prop][property] != undefined) {
 if (originalReference[prop][property] != null) {
 if (reference[prop][property] == null || reference[prop][property] == '') {
 // Assign old non-null value to new object if new value is empty or null
 reference[prop][property] = originalReference[prop][property];
 }
 }
 }
 }
 }
 // Check for array
 if (Array.isArray(reference[prop]) && typeof Array.isArray(originalReference[prop])) {
 // Recurse if both are arrays
 reference[prop].forEach((item, index) => vm.evaluateEmptyValues(item, originalReference[prop][index]));
 }
 } else {
 if (originalReference[prop] != undefined) {
 if (originalReference[prop] != null) {
 // Assign original value to new object
 reference[prop] = originalReference[prop];
 }
 }
 }
 }
 } catch(err) {
 console.log(err);
 }
 }
},

• Simon Forsberg: What is your use-case for this? What is the bigger picture?
• legoMyEgo: I load an object from a table onto my page. There's a button at the top that makes an API call to the ISBNdb. The API comes back in another structure, so I rebuild a new object from the API data, to have the same fields as the loaded object from the table and new values from the API data. Then, I funnel that new object and the old loaded object through this above function, to ensure that any of the values from API data didn't come back null or empty, (because it could potentially overwrite a non-empty value from the old loaded object with that empty value coming in from the API)
• Blindman67: if (reference[prop] != undefined || reference[prop] != null) { is never false, making the last else redundant. If your code works then 9 lines can be removed without effect. Along with 4 lines of the try and catch, and the one line var length = Object.keys(reference[prop]).length; length is never used, you have 12 out of 39 lines of code that does nothing. You should fix the code if you want a good review.

Answer 1

Very bad code

Your code is very poorly written making it difficult to workout what your intent is.

The try catch was a red flag and immediately told me not to trust the code.

Looking further at your code I found it full of redundant code and conflicting logic.

List of problems

• if (reference[prop] != undefined || reference[prop] != null) {

  Is the same as if(A == true || A == false) {

  This is always true, if A is not true, then it is true that A is false

  That makes the else and the associated block redundant (will never happen)

• if ( /*...*/ && typeof Array.isArray(originalReference[prop])) {

  typeof creates a non empty string. Thus the second half of the above statement is always true and thus redundant.

• if (typeof(reference[prop]) == 'object' && typeof(originalReference[prop]) == 'object') and

  if (Array.isArray(reference[prop]) && typeof Array.isArray(originalReference[prop])) {

  If reference[prop] is an array, null, or an object and originalReference[prop] is an array, null, or object then both the above statements are true, which I think is not your intent.

• var length = Object.keys(reference[prop]).length;

  length is never used making this line redundant.

• reference[prop].forEach((item, index) => vm.evaluateEmptyValues(item, originalReference[prop][index]))

  It is common for objects to contain references to themselves, or have properties that reference them selves. Thus this type of recursion can end up in an infinite loop. Luckily JS has a finite call stack that will cause an exception to be thrown, but as you catch the errors rather than stop, it will grind to a stop as it throws overflow error after error till it gets past all the self references.

Minor points

• var vm = this;

  The reference to vm is used in the previous point. The forEach is using an arrow function that maintains the outer context of this. The variable vm is thus redundant.

• referenceLength = Object.entries(reference).length;

  Object.entries creates an array of arrays. [[key, value], [key, value], ...] You should use Object.keys or Object.values if you want the number of properties.

• typeof (reference[prop]) == 'string'

  typeof is a token not a function and does not need to be followed by a pair of ()

• Use === or !== rather than == or !=

Comments just add to the mess

Your comments are noise if they repeat what is self evident in the code.

Or worse, comments are lies when in direct conflict with the code

You should only comment when the code can not represent the higher level abstraction that the logic is implementing. This is usually rare.

The better the code the less comments are needed to understand what is going on.

Some examples

• // Do another loop

  for (var property in reference[prop]) {

  Really!... a loop you say.

• // Check if current prop in both objects is an object

  if (typeof(reference[prop]) == 'object' && typeof(originalReference[prop]) == 'object') {

  Again the comment is repeating what is self evident in the code.

• if (Array.isArray(reference[prop]) && typeof Array.isArray(originalReference[prop])) {

  // Recurse if both are arrays

  No its not?... The comment is in direct conflict the the statement above it.

Summing up

Sorry to be so harsh, but your code is boarder line on topic for code review. You need to be much more careful when writing code.

I think part of the problem is the very long naming and indirect referencing (eg originalReference[prop][property]) you are using which is making it hard to see the logic from the referencing.

Your code is nested up to 10 times, and one line is over 120 characters long. In that line of the 100 characters (ignoring indent/white spaces) 71 characters are naming, and only 29 are part of the logic. Little wonder you are making obvious logic mistakes in the code, you can not see the wood from the trees.

There are a variety of other problems in your code that I did not address.

• legoMyEgo: Thank you! The harsher the better, I only recently graduated college and started working, so this sort of feedback will make me a better coder in the future projects
• Sᴀᴍ Onᴇᴌᴀ: I feel like the term "redundant" is overloaded, i.e. used to mean different things... e.g. in "That makes the else and the associated block redundant (will never happen)" I would have used the word "unreachable" instead of "redundant"; Then for "length is never used making this line redundant." I would have used a word like "superfluous" or "useless" instead of "redundant"