Is Wherefore Art Thou solution actually wrong?

1

So i was reviewing the code i wrote for Intermediate Algorithm Scripting section. When i do this i tend to go to github repository of FCC and check what kind of answers they came up with. This one was very similar to the one i wrote, but there is one minor difference which changes the results a bit.

Now i should mention that English is not my first language, therefore there is room for misunderstanding, the same goes for JavaScript in general so i might be very wrong.

This is the function i wrote:

function myFunction(collection, source){
  properties = Object.keys(source);
  
  return collection.filter(function(item){
    return properties.reduce(function(total,property){
      //line below is in question
      return item[property] === source[property] && item.hasOwnProperty(property) && total;
    }, true);
  });
}

Here is solution from freecodecamp github repository:

function whatIsInAName(collection, source) {
  var srcKeys = Object.keys(source);

  return collection.filter(function (obj) {
    return srcKeys.reduce(function (res, key) {
      //line below is in question
      return obj.hasOwnProperty(key) && obj[key] === source[key];
    }, false);
  });
}
Summary

Make a function that looks through an array of objects (first argument) and returns an array of all objects that have matching property AND value pairs (second argument). Each property and value pair of the source object has to be present in the object from the collection if it is to be included in the returned array.

Now both pass the test on FCC, but the results are different when you consider some other test cases which in my opinion better represent what the challenge was looking for.

extra test case 1:

collection:

[{ "a": 1, "b": 2 }, { "a": 1 }, { "a": 10, "b": 2, "c": 2 }]

source:

 { "a":1, "c": 2 }

In this case FCC function considers third object of collection to be matching with source object. That is however not true since parameter ‘a’ has value 10, not 1. My function returns empty array as expected.

extra test case 2:

collection:

[{ "a": 1, "b": 2 }, { "a": 1 }, { "a": 1, "b": 2, "c": 2 }]

source:

{ "b":2, "a": 1 }

Here FCC function returns an array which consists of all collection objects. That is however wrong result since second object of collection doesn’t have parameter ‘b’.

Conclusion:
I may be really bad at understanding English, confused or tired, but it doesn’t look right to me.

This line in FCC code does not take into consideration previous result, so it will return Boolean coming from logical operation performed only on last element of keys array, while simply ignoring previous ones. Also initial value is set to ‘false’.

return obj.hasOwnProperty(key) && obj[key] === source[key];

My code takes into consideration results of previous logical operations. Init value is set to true.

return item[property] === source[property] && item.hasOwnProperty(property) && total;

Jsbin with mind boggling amounts of console.logs:

2

Yes, you are correct here. By the nature of reduce function if the previous value is not used (res in their case) the overall result of the function call is the last instance of callback(), that is obj.hasOwnProperty(lastKey) && obj[lastKey] === source[lastKey]

In test case 1 last key-value pair from source (c:2) passes the callback check => the incorrect solution considers it enough for the whole { “a”: 10, “b”: 2, “c”: 2 } object.

In test case 2 every object from the collection includes a:1 pair, so every object passes.

Good on you for finding a mistake in the official FCC solution. The next step would be to create an Issue or even submit a pull request to the repo.

I also like how you used reduce to solve it. I did it a bit differently. Would you be interested to take a look?

function myName(collection, source) {
  var sourceKeys = Object.keys(source);

  return collection.filter(function(obj){
    return sourceKeys.every(function(k){
      return source[k] === obj[k];
    });
  });
}
3

Your function passes my test cases perfectly. It also looks way more readable than mine. I wasn’t really familiar with .every(). Started to learn JS quite recently. Very elegant solution!

4

Using reduce is interesting, but .every() or .some() are, I think, clearer solutions, given what they return.

If you use fat arrows, you can reduce (ha!) that down to …

function myName(collection, source) {

   const sourceKeys = Object.keys(source);

   return collection.filter( ( obj ) => sourceKeys.every( ( k ) => source[ k ] === obj[ k ] ) );

}

Which I think is very readable, so long as you know what every() does.

My solution was very similar to yours, so I may be biased. :slight_smile: But I definitely wouldn’t have thought to use reduce to solve a problem of filtration!

5

.reduce() just popped up due to Condense arrays with reduce challenge. Then i didn’t really think about alternatives :slight_smile:.

6

Yes, I didn’t mean that as a criticism! Reduce is actually a method I should explore more - I don’t use it much, although now that I think of it there are a number of places I can’t use map but instead use forEach while accumulating to an array which would be perfect situations to use reduce instead. :slight_smile:

7

you guys seem miles ahead of me in your understanding. I just did this one in the last few days too and my solution looks nothing like any of yours!

Can you see any holes in mine?

Commented here

function whatIsInAName(collection, source) {
  var arr = [];
  collection.filter(doesItMatch);
  function doesItMatch(collObj) {
  	var flag = 0;
    for (var sourceProp in source) {
    	for (var collProp in collObj){
        if ( sourceProp + source[sourceProp] === collProp  +  collObj[collProp]){
        	flag++;
        	if (flag == Object.keys(source).length){
        		arr.push(collObj);
        	}
        }
      }
    }
 }
  return arr;
}

Congrats on finding the bug too @GrayTable

8

Good overall. Allow me to point out a few ways to improve it:

First, you are using Array.filter like Array.forEach, no need really.

Second you are counting on string concatenation for key-value comparison, which will produce false positives in cases of ambiguity such as just a string or a number coerced to string:
Compare two objects source = {a: 1} andcollObj = {a: "1"}. They are obviously different, but sourceProp + source[sourceProp] === collProp + collObj[collProp] will return true, since "a"+1 === "a"+"1"
You should use (sourceProp === collProp && source[sourceProp] === collObj[collProp]) instead.

Third, there is really no need to recalculate Object.keys(source).length every time, once at the very top of the function together with var arr = []; would be enough.

Fourth?, some would suggest an extra condition of sourceProp.hasOwnProperty(sourceProp) inside each loop to skip prototype properties, but that’s not necessary in this case(while a good rule of thumb in general) as there are no [Enumerable] properties on these objects’ prototypes.

1 Like
9

I’ll go and educate myself in the difference :blush:

Now you’ve said that, yes I can see that clearly… error on my part that shouldn’t be there as I know how concatenation works v && operators, I’ve written plenty enough of each now.

don’t know why I didn’t! that was the last line I wrote, it worked (after hours of trying) and i got excited haha

I thought my for…in loop was only itterating enumerable properties anyway? I shall go and read-up there as well.

Thank you for your detailed answer, Its much appreciated :thumbsup:

10

On the last point, yes, yes you are absolute right that only enumerable properties get iterated over, I just meant that inclusion of hasOwnProperty in a for...in loop is such a staple “just in case object extends some other object”, that you may consider adding that in a more general case when comparing one object to another, though it isn’t necessary here. Rule of thumb that some would suggest you follow… Not me, not here :slight_smile:

11

So if it wasn’t you, or here, then including the check would be concidered good practice :smiley: