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);
        }
    }
},

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) — legoMyEgo
– legoMyEgo, Commented Apr 17, 2019 at 15:25
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. — Blindman67
– Blindman67, Commented Apr 17, 2019 at 16:39

Sᴀᴍ Onᴇᴌᴀ · Accepted Answer · 2019-04-19 23:18:13Z

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.

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 — legoMyEgo
– legoMyEgo, Commented Apr 18, 2019 at 13:05
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" — Sᴀᴍ Onᴇᴌᴀ
– Sᴀᴍ Onᴇᴌᴀ ♦, Commented Apr 19, 2019 at 23:11

Stack Exchange Network

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

1 Answer 1

Very bad code

List of problems

Minor points

Comments just add to the mess

Summing up

You must log in to answer this question.

Hot Network Questions

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

1 Answer 1

Very bad code

List of problems

Minor points

Comments just add to the mess

Summing up

You must log in to answer this question.

Related

Hot Network Questions