Skip to content

Guide section 5.1 on object destructuring may lead to anti-pattern #2619

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

Open
pedropedruzzi opened this issue Jul 26, 2022 · 3 comments
Open

Comments

@pedropedruzzi
Copy link

This issue is realted to guide section 5.1 Use object destructuring when accessing and using multiple properties of an object

Although the section rationale and examples are very clear and logical, IMO when followed unrestrictedly (or naively) it may lead to an anti-pattern with the bad features this very section is trying to avoid in the first place (repetitive code, opportunities for mistakes and unnecessary extra temporary references).

Apparently the anti-pattern arises more clearly when there is a combination of these conditions:

  • Accessed object properties have meaningful names and are each only referenced once, thus not requiring local variables.
  • There are more than a handful of accessed properties
  • The accessed properties are mostly used to create a new object, with or without key/value transformations

Here's an example to illustrate:

// bad
function toNewUserType(user) {
  const {
    id: userId,
    status: userStatus,
    email,
    phone,
    encryptedPassword,
    firstName,
    middleName,
    lastName,
    jobTitle,
    birthDate,
    createdAt,
    updatedAt,
    address,
  } = user;

  return {
    userId,
    userStatus,
    email,
    phone,
    encryptedPassword,
    jobTitle,
    birthDate,
    createdAt: toDate(createdAt),
    updatedAt: toDate(updatedAt),
    address: toNewAddressType(address),
    userHash: idToHash(userId),
    fullName: getFullName(firstName, middleName, lastName),
  };
}

// still bad
function toNewUserType({
  id: userId,
  status: userStatus,
  email,
  phone,
  encryptedPassword,
  firstName,
  middleName,
  lastName,
  jobTitle,
  birthDate,
  createdAt,
  updatedAt,
  address,
}) {
  return {
    userId,
    userStatus,
    email,
    phone,
    encryptedPassword,
    jobTitle,
    birthDate,
    createdAt: toDate(createdAt),
    updatedAt: toDate(updatedAt),
    address: toNewAddressType(address),
    userHash: idToHash(userId),
    fullName: getFullName(firstName, middleName, lastName),
  };
}

// good
function toNewUserType(user) {
  return {
    userId: user.id,
    userStatus: user.status,
    email: user.email,
    phone: user.phone,
    encryptedPassword: user.encryptedPassword,
    jobTitle: user.jobTitle,
    birthDate: user.birthDate,
    createdAt: toDate(user.createdAt),
    updatedAt: toDate(user.updatedAt),
    address: toNewAddressType(user.address),
    userHash: idToHash(user.id),
    fullName: getFullName(user.firstName, user.middleName, user.lastName),
  };
}

If the package owners agree with the problem, I believe we should be able to add some content to warn about this anti-pattern and how to avoid it.

Thanks in advance.

@ljharb
Copy link
Collaborator

ljharb commented Jul 26, 2022

I’m not sure this is an antipattern, given the language features available to us.

Just because there’s repetition doesn’t automatically mean it’s bad - it would be nice to reduce it, but in your last example, user is repeated many many times. The repetition of de and re structuring is imo much better than the repetition of the object.

@pedropedruzzi
Copy link
Author

pedropedruzzi commented Jul 26, 2022

I agree with "Just because there’s repetition doesn’t automatically mean it’s bad". This issue is really to raise this discussion of whether or not this is an antipattern (should be marked with "question" label?). So thanks for your input!

I disagree on your assessment on the two types of repetition though. Mostly because the user.field repetition is purely syntactical (a future language feature may even remove its need - e.g. a new object to object assignment with a syntax similar to destructuring) and limited to a single line per property. Whereas the de/re structuring requires two non-contiguous lines per property and, as with any "regular" case of duplication, there are more opportunities for mistakes.

@lahari936
Copy link

Thanks, @pedropedruzzi, for raising this interesting point and providing clear examples. It highlights a scenario where the strict application of guideline 5.1 might lead to code that some developers find less readable or more verbose than an alternative.

You're focusing on a specific pattern: transforming/mapping one object structure to another, involving many properties, where each input property is typically used only once in the output.

Let's break down the trade-offs in this context:

Destructuring Approach (as per 5.1 / your "bad" examples):

  • Pros: Explicitly declares input dependencies upfront, avoids user. repetition in the function body, provides local variables (useful if usage expands later), allows easy renaming.
  • Cons (in this specific mapping scenario): Can lead to listing properties twice (destructuring + return literal), increased verbosity for many properties, potential cognitive load mapping destructured names to usage, risk of mismatches.

Direct Access Approach (your "good" example):

  • Pros: Often more concise for direct mappings, references each user.property only once where it's used, can be easier to follow the transformation logic inline.
  • Cons: Repeats the user. identifier frequently (visual noise for some), less explicit dependency declaration, less idiomatic if destructuring is heavily used elsewhere.

Is it an Anti-Pattern?

Labeling it a strict "anti-pattern" might be too strong, as @ljharb pointed out, readability and preference for types of repetition can be subjective. The repetition of user. is also a factor some developers actively avoid using destructuring.

However, your point is valid: in the specific case of mapping many properties directly into a new object literal with minimal logic beyond simple renaming or transformation, the boilerplate of destructuring + re-structuring can outweigh its benefits in terms of clarity and conciseness compared to direct user.property access within the returned object literal.

Suggestion for the Guide:

Perhaps Section 5.1 could benefit from a brief note acknowledging this trade-off, without fundamentally changing the core guideline. Something like:

"While destructuring is generally preferred for accessing multiple properties, consider the trade-offs when dealing with functions that primarily map a large number of properties from one object structure to a new object literal. If most properties are used only once for this mapping, directly accessing object.property within the new object literal might occasionally offer better conciseness and readability, though this comes at the cost of repeating the object identifier. Evaluate based on the number of properties, the complexity of transformations, and team conventions."

This acknowledges the nuance you've highlighted without creating a complex exception, emphasizing developer judgment.

This seems like a valuable discussion point for the maintainers regarding whether such a clarification would enhance the guide's practical applicability.

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

No branches or pull requests

4 participants
@ljharb @pedropedruzzi @lahari936 and others