GiftForGood / website

https://www.giftforgood.io
MIT License
4 stars 5 forks source link

[RFC] Backend Refactor #447

Open jinyingtan opened 3 years ago

jinyingtan commented 3 years ago

Motivation

The main issue is that there are quite a few duplicated logic that is all around. This makes the code harder to maintain further down the road if a bug or a new function would require the same logic. In the scenario of a bug happening the shared logic , we will have to make changes in several places.

Some of the scenario where logic are duplicated:

Proposed Changes

Duplicated Logic

To reduce duplicated logic, common functions like uploading of images should be abstracted to a common folder. On top of that, we can also abstract logic that is not related to an API to the common folder to prevent further duplication of code in the future.

Standardized error handling

Currently, errors are handled both on the API level and also within helper function level. We can look to return error messages in the helper function and let the API level function throw the error.

Standardized error messages

Currently, each API is returning their own custom error message. Also, FE is directly using the error message return from the API calls. We can look into shift all error messages to a file and have more user-orientated error messages.

Remove the custom type error message

Since FE only uses error codes and does not check for the type of error (e.g. AuthError), we can collapse all of this to a common error that has error code and error message. This is so that we do not need to create a new error every time we create a new API.

Removing unused functions and APIs

Some functions and APIs are not used and we should remove them so that we don't get confused in the future.

Shift non-API file out of API folder

This is a specific case for time.js under the API folder. As this is not a call to our database, a folder under utils should be created for time.

Process for refactoring

Refactor one API at a time.

kohchihao commented 3 years ago

Looks good to me! I can help with the refactoring but maybe you show us how you want it to be like first.

kohchihao commented 3 years ago

Should we also standardise the error messages/code for cloud function? So that it is consistent on both sides @jinyingtan

jinyingtan commented 3 years ago

For the error messages,

For other stuff, generally just remove those non-related api functions into the common folder to prevent further duplication of code. Don't really have a straightforward way to do it.

jinyingtan commented 3 years ago

Yes we can also do it for cloud functions, but let's take it one step at a time and refactor this part first