0
\$\begingroup\$

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);
        }
    }
},
\$\endgroup\$
3
  • 1
    \$\begingroup\$ What is your use-case for this? What is the bigger picture? \$\endgroup\$ – Simon Forsberg Apr 17 '19 at 15:12
  • \$\begingroup\$ 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) \$\endgroup\$ – legoMyEgo Apr 17 '19 at 15:25
  • \$\begingroup\$ 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. \$\endgroup\$ – Blindman67 Apr 17 '19 at 16:39
2
\$\begingroup\$

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.

\$\endgroup\$
2
  • 1
    \$\begingroup\$ 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 \$\endgroup\$ – legoMyEgo Apr 18 '19 at 13:05
  • \$\begingroup\$ 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" \$\endgroup\$ – Sᴀᴍ Onᴇᴌᴀ Apr 19 '19 at 23:11

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.