I have different methods in my app to fetch users:
1. Fetch international users
2. Fetch users from same region
3. Fetch users from same city
Currently, I have implemented 1 method for each one:
const fetchIntenationalUsers = async (limit = 10) => {
const result = await db.collection("users").limit(limit).get();
...
return parseUsers(result);
}
const fetchSameRegionUsers = async (region, limit = 10) => {
const result = await db.collection("users").where("region", "==", region).limit(limit).get();
...
return parseUsers(result);
}
const fetchSameCityUsers = async (city, limit = 10) => {
const result = await db.collection("users").where("city", "==", city).limit(limit).get();
...
return parseUsers(result);
}
I have thought to create a generalized method like:
const fetchUsers = (queryType = "international", location = {}, limit = 10) => {
const queries = {
"international": db.collection("users"),
"region": db.collection("users").where("region", "==", location.region),
"city": db.collection("users").where("city", "==", location.city),
};
const result = await queries[queryType].limit(limit).get();
...
return parseUsers(result);
}
But I am not sure if this is an anti-pattern or a design pattern. Any other approach?
The general idea looks fine to me, but there's one issue: with this line:
const queries = {
"international": db.collection("users"),
"region": db.collection("users").where("region", "==", location.region),
"city": db.collection("users").where("city", "==", location.city),
};
You're calling all of those database methods when the object is created, not just the one you need. (You also aren't handling possible errors thrown by the unused methods.)
For a slight tweak, consider functions that return the queries instead. You can also do db.collection('users') up front.
const users = db.collection("users");
const queries = {
international: () => users,
region: () => users.where("region", "==", location.region),
city: () => users.where("city", "==", location.city),
};
const result = await queries[queryType]().limit(limit).get();
Yes, I think it's an antipattern to put them all together in a single "generalised" method, and introduce that queryType enum. You'll get more and more query types in the future, and it will create a huge mess in that god method. (Also, it has the disadvantages mentioned in @CertainPerformance's answer).
Instead, I recommend to keep the separate functions (that can be called separately), but abstract out the duplicated parts of the code into one abstract helper function:
async function fetchUsers(addCondition, limit = 10) {
const query = addCondition(db.collection("users")).limit(limit);
const result = await query.get();
…
return parseUsers(result);
}
export const fetchInternationalUsers = (limit) =>
fetchUsers(q => q, limit);
export const fetchSameRegionUsers = (region, limit) =>
fetchUsers(q => q.where("region", "==", region), limit);
export const fetchSameCityUsers = async (city, limit) =>
fetchUsers(q => q.where("city", "==", city), limit);