0
\$\begingroup\$

I have 2 dictionaries that I have to compare, if an element of one dictionary exists in the other dictionary, then add the element to an array but only one time...

here the working code

const getFavouriteShows = (graphql, favouriteShows) => {
    const arraya = [];

    //iterate trough favouriteShows
    for (let item in favouriteShows) {
      //iterate trough graphql
      for (let itemGQL in graphql) {
        //if same slug, add the item to new array JUST ONE TIME
          if(favouriteShows[item].slug === graphql[itemGQL].show?.slug){
            arraya.push(graphql[itemGQL])
            break
          }
      }
    }
       return Object.assign(arraya)
  }

So, how to express this in a more inline functional way? using map, filter.

Thanks!

Edit, examples for the data coming in and out https://gist.github.com/mako34/44301d3c96249de9a07aa2550ca1172a

\$\endgroup\$
2
  • \$\begingroup\$ Can you provide some example inputs and outputs? \$\endgroup\$ Commented Jan 19, 2021 at 4:33
  • 1
    \$\begingroup\$ The current question title, which states your concerns about the code, applies to too many questions on this site to be useful. The site standard is for the title to simply state the task accomplished by the code. Please see How do I ask a good question?. \$\endgroup\$ Commented Jan 19, 2021 at 8:27

1 Answer 1

2
\$\begingroup\$

Review

  • Use semicolons. Why? If you don't know why you should use them, then you should use them. Not using them can result in very hard to track down bugs that will waste hours of your valuable time.

  • Use const wherever and whenever possible.

  • The return expression Object.assign(arraya) does nothing and is not required ( Object.assign returns the first argument unchanged if there is only one argument). return arraya; will do the same.

  • Use for of rather than for in. When you use for in you need to ensure that the key belongs to the object which just makes the code more complex. If you dont check for own property Object.hasOwnProperty then you risk the code not working for reasons unrelated to your code.

    To get the keys of an object use Object.keys eg you can then replace the for in with for (let item of Object.keys(favouriteShows)) {

    As it is only the value slug you want from each item in the object favouriteShows you can further refine the for of loop to get that value for you. Use Object.values which will get the value you find with favouriteShows[item] and use destructure assignment to pull out slug thus (Note the const) you have for (const {slug} of Object.values(favouriteShows)) {

  • Avoid naming variables after what they are, rather name them for what they represent (Unless they represent unknowns). Eg arraya would be better as results

  • Don't use complicated or long names when there is no need. The arguments (graphql, favouriteShows) can be simplified to (graph, shows) without losing any meaning.

Problems

There are a number of problems with this code.

  1. The statement that matches shows by slug if(favouriteShows[item].slug === graphql[itemGQL].show?.slug){ will match any two shows that do not have slug defined IE slug === undefined

    This is not a problem if all objects always have slug defined. However the use of show?. makes it clear that this is at least not true in part.

    You can check if slug is defined in the outer loop so you don't waste time searching for undefined in the inner loop.

  2. In your question you state "...then add the element to an array but only one time." However if favouriteShows contains items that have the same value for slug and there is a match for that value in graphql your function will add the same entry for every occurrence in favouriteShows

    To prevent this problem you can use a Set to hold the result rather than an Array, It will ensure only one copy of each item is added.

Improvement

The inner loop repeats, stepping over the same items for every item in favouriteShows. You can use a Map that indexes items via a generated hash value. This reduces the complexity of your inner loop from \$O(n)\$ to \$O(1)\$.

Thus your code ends up as with added comments

// Keep the argument names simple
const favouriteShows = (graph, shows) => {

    // Use set to ensure only a single instance of each item will be in the result
    const result = new Set();  

    // Create a map of graph indexed by slug so the inner loop is not needed.
    const bySlug = new Map(Object.values(graph).map(val => [val.show?.slug, val]));

    // Extract value of slug from the item values in shows
    for (const {slug} of Object.values(shows)) {
        
        // Make sure slug is defined.
        // It is unclear if this value may be falsey so test against undefined
        if (slug !== undefined) {

            // Does the map bySlug have an entry that matches slug?
            if (bySlug.has(slug)) {

                // Yes then get that entry and add it to the result.
                result.add(bySlug.get(slug));
            }
        }
    }

    // To return an array convert the set to an array.
    return [...result.values()];
}

It can also be compacted a little, and making an assumption that slug will be either undefined or some truthy string we get

const favouriteShows = (graph, shows) => {
    const result = new Set();  
    const bySlug = new Map(Object.values(graph).map(val => [val.show?.slug, val]));
    for (const {slug} of Object.values(shows)) {
        const show = bySlug.get(slug);             
        slug && show && result.add(show);
    }
    return [...result.values()];
}

Note that bySlug.get will find the last entry in the object graph if graph contained duplicates (slug has the same value more than once). As your code uses the first instance you may want to change the second line in the function so that it reverses (via Array.reverse) the order that the map is created. If there are no duplicates then it does not matter.

const favouriteShows = (graph, shows) => {
    const result = new Set();  
    const bySlug = new Map(Object.values(graph)
       .map(val => [val.show?.slug, val])
       .reverse()
    );
    for (const {slug} of Object.values(shows)) {
        const show = bySlug.get(slug);             
        slug && show && result.add(show);
    }
    return [...result.values()];
}
\$\endgroup\$

You must log in to answer this question.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.