Filter an object array based on certain conditions

Question

How can I rewrite this in a cleaner way?

I have this function that takes a list of contacts of different types (are being called relationships. If there are contacts of type A the list should be filtered to only include contacts of type A, otherwise, it should only include contacts of type B.

const getContactNames = () => {
  let filteredContacts = contacts.filter(c => c.relationship === 'A')
  if (!filteredContacts.length) {
    filteredContacts = contacts.filter(
      c => c.relationship === 'B'
    )
  }
  return filteredContacts.map(contact => getContactName(contact))
}

The getContactName function:

const getContactName = contact =>
  `${contact?.title || ''} ${contact?.firstName || ''} ${
    contact?.lastName || ''
  }`.trim()

contact object:

const contact = {title:"Mr", firstName:"test", lastName: "test", relationship: "B", address: "test", city:"NY", state:"NY"}

How can I rewrite the getContactNames function in a cleaner way?

Answer 1

rewrite this in a cleaner way?

The contact object does all the work for itself: initializing properties, formatting name output. If some client code is doing something to an object's properties then it probably should be a method/function in that object.

function contact(fromJSON) {
       title: fromJSON.title     || "",
   firstName: fromJSON.firstName || "",
    lastName: fromJSON.lastName  || "",
relationship: fromJSON.relationship || "",

  contactName: function() { 
    return `${ this.title } ${ this.firstName } ${ this.lastName }`.trim()
  }
}

New contact objects are initialized with blank string. The chaining operator is not needed as there are no nested objects in contact

---

Filtering for "A else B" types. The point is avoiding nesting when practical which naturally makes reading and understanding easier.

Yes, filtering for "B"s may sometimes be unnecessary but I don't care. I love how clean, simple, and clear the code feels. If performance might be an issue, well, that's what testing is for.

// if there are no type "A" contacts, give me type "B"s
const getContacts = () => {
   let ContactsA = contacts.filter( c => c.relationship === 'A' )
   let ContactsB = contacts.filter( c => c.relationship === 'B' )
  
   return ContactsA.length ? ContactsA : 
          ContactsB.length ? ContactsB :
          []
}   

---

Client code

const contacts = // array of contacts from JSON object
const AorBcontacts = getContacts()
AorBcontacts.forEach( x => console.log( x.contactName() ))

Answer 2

I'll address the two functions independently. Since you've asked "how can I rewrite this in a cleaner way", I've assumed you're interested in the process of determining a cleaner way, not just the end result. So I've written out my thought process.

(By the way, in a few places I've said "I don't like X", which I'm using as a short hand for "I have an intuition that we can do better, but I haven't fully explored the tradeoffs, I'd like to try changing it and see if it's better". I acknowledge that what I personally like and don't like is irrelevant to you, but there are definitely times when intuition leads us to a new solution which we can objectively conclude is preferable.)

getContactNames

The first thing I would do is to write your code in English. This is the best way I know of to write readable code, because I'm good at assessing whether English is awkward.

function getContactNames() {
   // if there are any contacts with relationship A
   // use only the As
   // otherwise use only the Bs
   // return the contact names for whatever contacts you chose
}

I've noticed that you're filtering the list twice, but we don't have to. For a marginal performance improvement, and because I consider this to make the code simpler, I would suggest looping once instead (and assigning to the appropriate list in the loop).

function getContactNames() {
   const typeAContacts = [];
   const typeBContacts = [];
   // for each of the contacts, put them into the correct contact list
   // if there are any type A contacts, use those
   // otherwise, use the type B contacts
   // return the contact names for whatever contacts you chose
}

The next thing I'd like to address is that this method is doing two things (choosing the contact list; mapping to names). I'll split the method:

function getContactNames() {
   const filteredContacts = chooseContactsOfOneMostAppropriateType(contacts);
   return filteredContacts.map(getContactName);
}

// This method name I chose is awkward, but it's a placeholder for now
function chooseContactsOfOneMostAppropriateType(contacts) {
   const typeAContacts = [];
   const typeBContacts = [];
   // for each of the contacts, put them into the correct contact list
   // if there are any type A contacts, return those
   // otherwise, return the type B contacts
}

I wrote filteredContacts.map(getContactName) instead of filteredContacts.map(contact => getContactName(contact)) because they are functionally equivalent but the former is more concise. It's a style preference though.

OK, I've finished with getContactNames and will now focus on the private chooseContactsOfOneMostAppropriateType method:

function chooseContactsOfOneMostAppropriateType(contacts) {
   const typeAContacts = [];
   const typeBContacts = [];
   contacts.forEach(contact => {
     if (contact.relationship === 'A') {
       typeAContacts.push(contact);
     } else if (contact.relationship === 'B') {
       typeBContacts.push(contact);
     }
   });
   return typeAContacts.length ? typeAContacts : typeBContacts;
}

Decisions like using the ternary expression and the format of the if/else should match whatever your project style guide specifies. Some teams may prefer you omit the curly braces on the one line if/else, or use switch. Other teams may disallow the use of ternary expressions, so you would need another if/else.

At this point, I don't like how long this is and may choose to split it:

function chooseContactsOfOneMostAppropriateType(contacts) {
   const { B: typeBContacts, A: typeAContacts } = groupByRelationship(contacts);
   return typeAContacts.length ? typeAContacts : typeBContacts;
}

function groupByRelationship(contacts) {
   return contacts.reduce((all, one) => {
     all[one.relationship] = all[one.relationship] || [];
     all[one.relationship].push(one);
     return all;
   }, {});
}

Since groupByRelationship is very generic, I would extract it to a separate file and unit test it independently.

getContactName

I prefer the function syntax over anonymous functions. When debugging, this function is named, whereas the anonymous function assigned to a constant is not a named function. However, it's mostly a team preference thing. I'd say go with whatever is conventional in your context.

function getContactName(contact) {
  return `${contact?.title || ''} ${contact?.firstName || ''} ${
    contact?.lastName || ''
  }`.trim()
}

First, to English

function getContactName(contact) {
  // return the title, first name, and last name (if present) joined by spaces
}

I like to use filter/join for this sort of thing because it's closer to the English version:

function getContactName(contact) {
  // return the tile, first name, and last name
  const { title, firstName, lastName } = contact;
  const nameParts = [title, firstName, lastName];
  return nameParts
    // if present
    .filter(x => !!x)
    // joined by spaces
    .join(' ');
}

• .filter(x => !!x) is equivalent to || ''
• join(' ') is equivalent to the } ${ between the terms in the original code.

function getContactName(contact) {
  const { title, firstName, lastName } = contact;
  const nameParts = [title, firstName, lastName];
  return nameParts.filter(x => !!x).join(' ');
}

Putting it all together:

function getContactNames() {
   const filteredContacts = chooseContactsOfOneMostAppropriateType(contacts);
   return filteredContacts.map(getContactName);
}

function chooseContactsOfOneMostAppropriateType(contacts) {
   const { B: typeBContacts, A: typeAContacts } = groupByRelationship(contacts);
   return typeAContacts.length ? typeAContacts : typeBContacts;
}

function groupByRelationship(contacts) {
   return contacts.reduce((all, one) => {
     all[one.relationship] = all[one.relationship] || [];
     all[one.relationship].push(one);
     return all;
   }, {});
}
function getContactName(contact) {
  const { title, firstName, lastName } = contact;
  const nameParts = [title, firstName, lastName];
  return nameParts.filter(x => !!x).join(' ');
}

We now have some generic contact stuff and some thing specific to this code you're working on. I would extract groupByRelationship and getContactName into a separate contact utility, and leave getContactNames and chooseContactsOfOneMostAppropriateType in the current spot. Also, since chooseContactsOfOneMostAppropriateType is now so short, I think we could inline it. Thus:

// main code
function getContactNames() {
   const { B: typeBContacts, A: typeAContacts } = groupByRelationship(contacts);
   const filteredContacts = typeAContacts.length ? typeAContacts : typeBContacts;
   return filteredContacts.map(getContactName);
}

// contactManagement.js (or something like that)
function groupByRelationship(contacts) {
   return contacts.reduce((all, one) => {
     all[one.relationship] = all[one.relationship] || [];
     all[one.relationship].push(one);
     return all;
   }, {});
}
function getContactName(contact) {
  const { title, firstName, lastName } = contact;
  const nameParts = [title, firstName, lastName];
  return nameParts.filter(x => !!x).join(' ');
}