Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Simplify implementation of extractUniqueTagsObject() #294

Conversation

stuarteberg
Copy link
Contributor

@stuarteberg stuarteberg commented Aug 27, 2024

Here's my suggested implementation, inspired in part from the discussion in the code review meeting.

  • Guard clause to avoid deep indentation
  • Perform deduplication using Set instead of explicit includes check.
  • I assume validTagKeys.includes(key) is preferable to validTagKeys.some(...), but maybe I'm missing something.
  • I removed the global tagKeyNames, since I think it's clearer to the reader to see its definition inline.
  • Use the "make this an array if it isn't already" trick within the loop, too. Reduces the number of if/else blocks.
    • As discussed in the meeting, whether to use a new variable name or reuse the old one is a stylistic choice. Your call, of course.

I tested this implementation in my browser's dev console:

test setup
function extractUniqueTagsObject(contentCollection) {
    const uniqueTags = {};
    const validTagKeys = Object.keys(validTagsList);

  // For each key, determine the union of all unique values associated with it
  // (across all content items), excluding keys which aren't in validTagsList.
  contentCollection = Array.isArray(contentCollection) ? contentCollection : [contentCollection];
  contentCollection.forEach((contentItem) => {
    Object.entries(contentItem.data).forEach(([key, tagValues]) => {
      if (!validTagKeys.includes(key) || !tagValues || !tagValues.length) {
        return;
      }
      uniqueTags[key] ??= new Set();
      tagValues = Array.isArray(tagValues) ? tagValues : [tagValues];
      tagValues.forEach((tv) => {
        uniqueTags[key].add(tv);
      });
    });
  });

  // Convert from Sets to sorted Arrays
  Object.keys(uniqueTags).forEach((key) => {
    uniqueTags[key] = Array.from(uniqueTags[key]).sort();
  });
  return uniqueTags;
}

validTagsList = {
  pet: 'foo',
  robot: 'bar'
}
contentCollection = [
  {
    data: {
      invalid: 'invalid',
      pet: ['dog', 'cat'],
      robot: 'vacuum'
    }
  },
  {
    data: {
      pet: 'cat',
      robot: ['vacuum', 'flyflipper']
    }
  }
];result = extractUniqueTagsObject(contentCollection);
console.log(JSON.stringify(result, null, 2));

It produces the following output:

{
  "pet": [
    "cat",
    "dog"
  ],
  "robot": [
    "flyflipper",
    "vacuum"
  ]
}

Copy link

🎉 Thank you for your contribution!

The website maintainers will review your PR soon.

@stuarteberg stuarteberg force-pushed the simplify-extractUniqueTagsObject branch 2 times, most recently from d564c22 to e7637d5 Compare August 27, 2024 17:16
@stuarteberg stuarteberg force-pushed the simplify-extractUniqueTagsObject branch from e7637d5 to 97a8b2d Compare August 27, 2024 17:36
Copy link
Collaborator

@allison-truhlar allison-truhlar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for working on this, Stuart. The extractUniqueTagsObject function is much more readable now!

@allison-truhlar allison-truhlar merged commit d704a20 into JaneliaSciComp:main Aug 27, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants