ORIGINAL CODE
Imagine this spaghetti method:
const useDeleteMedia = (media, { onSuccess, onError } = {}) => {
const isMounted = useIsMounted();
const [loading, setLoading] = useState(false);
const [error, setError] = useState(null);
const deleteMedia = async () => {
try {
if (media.type === "audio") {
await api.audios.delete();
} else if (media.type === "video") {
await api.videos.delete();
} else if (media.type === "image") {
await api.images.delete();
} else {
throw new Error("Invalid media type");
}
onSuccess?.();
} catch (err) {
if (!isMounted) return;
setError(err);
onError?.(err);
}
};
return { error, deleteMedia };
};
REFACTORING USING OBJECT (FACTORY?)
In order to avoid the conditional chain etc. you may think about using some kind of factory of actions like:
const deleteAPI = Object.freeze({
audio: api.audios.delete,
video: api.videos.delete,
image: api.images.delete,
});
I am not sure if this is considered an “object factory” (I know what a factory function is, but if I don’t remember bad, I heard about the “object factory pattern” some time ago).
And then refactor the original code to something like:
const useDeleteMedia = (media, { onSuccess, onError } = {}) => {
const isMounted = useIsMounted();
const [loading, setLoading] = useState(false);
const [error, setError] = useState(null);
const deleteMedia = async () => {
try {
await deleteAPI[media.type]();
onSuccess?.();
} catch (err) {
if (!isMounted) return;
setError(err);
onError?.(err);
}
};
return { error, deleteMedia };
};
IS GENERALIZING BAD (sometimes)?
Another option could be to just make 3 different methods, in order to be more “flexible” and do different things in the future.
For example, imagine that after deleting each type of media, you have to update a context, decreasing “totalVideos”, “totalAudios”, or “totalImages”, depending on the media type.
This might look like:
const useDeleteMedia = (media, { onSuccess, onError } = {}) => {
const isMounted = useIsMounted();
const currentUser = useCurrentUser();
const [loading, setLoading] = useState(false);
const [error, setError] = useState(null);
const decreaseUserMedia = () => {
currentUser.setData((prevData) => {
if (media.type === "audio") {
return {
...prevData,
totalAudios: Math.max(prevData.totalAudios + 1, 0),
};
} else if (media.type === "video") {
return {
...prevData,
totalVideos: Math.max(prevData.totalVideos + 1, 0),
};
} else if (media.type === "audio") {
return {
...prevData,
totalImages: Math.max(prevData.totalImages + 1, 0),
};
}
});
};
const deleteMedia = async () => {
try {
await deleteAPI[media.type]();
onSuccess?.();
} catch (err) {
if (!isMounted) return;
setError(err);
onError?.(err);
}
};
return { error, deleteMedia };
};
Then the spaghetti code comes again. And you may think: well, then just compute the keys like:
const decreaseUserMedia = () => {
currentUser.setData((prevData) => {
const computedKey = `total${capitalize(media.type)}`;
return {
...prevData,
[computedKey]: Math.max(prevData[computedKey] + 1, 0),
};
});
};
And that looks pro, yeah, but what if I keys could not be computed that simple? What if I need to add more stuff in the future? What if I only want to do different side effects for each type of media?
SPLITTING CODE
In the other hand, I can just make different methods: useDeleteAudio()
, useDeleteVideo()
and useDeleteImage()
.
With this, I am repeating my self. BUT… I have more flexibility in order to add new features for each different type of media.
Also, there is no need to compute keys, or using helpers like capitalize()
.
QUESTION
Is it considered, in terms of code quality, to repeat ourselves in this type of situation? Is there any pattern that can solve my life here?