Destructuring with map and assigning to a constant

Question

I don't know exactly what to name this, but I was refactoring a code snippet and looking at this constant users I thought I'd do this destructuring:

Before:

async queryDBUsers({ commit }) {
 await getDocs(collection(db, "users"))
 .then((querySnapshot) => {
 const users = querySnapshot.docs.map((doc) => ({
 id: doc.id,
 name: doc.data().name,
 shelfName: doc.data().shelfName,
 photoURL: doc.data().photoURL,
 }));
 commit("setUsers", users);
 })
 .catch((error) => console.error(error));
},

After:

async queryDBUsers({ commit }) {
 await getDocs(collection(db, "users"))
 .then((querySnapshot) => {
 const users = querySnapshot.docs.map((doc) => {
 const [id, { name, shelfName, photoURL }] = [doc.id, doc.data()];
 return { id, name, shelfName, photoURL };
 });
 commit("setUsers", users);
 })
 .catch((error) => console.error(error));
},

The const users ends the function in exactly the same way in both code snippets.

My question is: is this good code practice? Or should I leave it the way it was?

• ARNON: To be honest what I'm finding really weird is having a const inside another and still having a return.
• slepic: To do this, you had to create an array first on the right side only to destructure it on the left side. I think this is sign that the destructuring is being abused. Also unless you're trying to omit something from the data I'd consider just returning {id:doc.id, ...doc.data()}.
• ARNON: In this given example I don't want to omit anything so {id:doc.id, ...doc.data()} fits very well. But there are snippets very similar to this one where doc.data() has a lot more than I need.

Answer 1

Super short review;

• Logging to console seems very primitive, consider a logging solution that handles different logging levels.
• { commit } is very bothering, why should a query function need to commit? Is it more a next function? Does it actually write and commit users? A comment would have been useful there.
• Actually, I am not sure why you restrict the fields in this function, which has a super general/bland name (queryDBUsers). Why not pass all the fields?
• For the destructuring, I think it's fine either way.

• ARNON: I agree with console being primitive; I am working with Vuex, that's why the { commit }; Sometimes I query the users in DB and sometimes the users in LocalStorage, it explains the boring queryDBUsers name; Thanks for your review. I really appreciate it.

Answer 2

Just one small suggestion regarding the deconstruction: You can deconstruct function parameters, so it could be written as:

.map(({id, data}) => [id, data()])
.map(([id, { name, shelfName, photoURL }]) => ({ id, name, shelfName, photoURL }))

• ARNON: That way the code was very clean! But I'm not sure doing two iterations would be faster.

Answer 3

You asked about destructuring so I am going to review the destructuring bit only.

Your after code looks fine. It's slighlty unconventional to destructure two separate things from a tuple you created. Instead, usually you see people do what @RoToRa suggested: destructuring in the function params first and then destructuring further in the function body.

Here's your after code:

async queryDBUsers({ commit }) {
 await getDocs(collection(db, "users"))
 .then((querySnapshot) => {
 const users = querySnapshot.docs.map((doc) => {
 const [id, { name, shelfName, photoURL }] = [doc.id, doc.data()];
 return { id, name, shelfName, photoURL };
 });
 commit("setUsers", users);
 })
 .catch((error) => console.error(error));
},

Now here's the slight modification:

async queryDBUsers({ commit }) {
 await getDocs(collection(db, "users"))
 .then((querySnapshot) => {
 const users = querySnapshot.docs.map(({id, data }) => {
 const { name, shelfName, photoURL } = data(); // if data is a func we should invoke it here and not in the map params
 return { id, name, shelfName, photoURL };
 });
 commit("setUsers", users);
 })
 .catch((error) => console.error(error));
},

This is typically what you will see. The benefit of doing it this way with the awkward const thrown in there is it's actually shorter and more writable. If the number of properties you are using from doc.data() or doc increases it becomes a pain to add doc.data().newProperty for each property you need. Also, if you wanted to change from doc.data() to doc.data.data() you would have to update each line (depending on your text editor that may not be so painful, still you want to minimize the places you touch when making changes). With destructuring, you would only have to change the const line from:

const { name, shelfName, photoURL } = data();

to this:

const { name, shelfName, photoURL } = data.data();

To answer your other questions:

is this good code practice? Or should I leave it the way it was?

The practice of destructuring and using Object Property Value Shorthand became a good practice as of ES6. It makes it easier to write and read. Leaving it the way it was ultimately is not an issue nor is it a "bad" practice. Rather, it's less common these days and I rarely see it implemented that way.