Skip to content
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

[RFC] Backend Refactor #447

Open
jinyingtan opened this issue Mar 6, 2021 · 4 comments
Open

[RFC] Backend Refactor #447

jinyingtan opened this issue Mar 6, 2021 · 4 comments

Comments

@jinyingtan
Copy link
Contributor

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:

  • Common function like uploading images are in multiple places
  • APIs are created duplicated code from other APIs
    • E.g. Auth is calling some user related logic and those logic could be shifted to user API instead.
      Other than this main issue, there are other small parts of the code that could be improved and it will be listed under proposed changes

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
Copy link
Contributor

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

@kohchihao
Copy link
Contributor

kohchihao commented Mar 7, 2021

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

@jinyingtan
Copy link
Contributor Author

For the error messages,

  • putting all the error messages into one
  • restructure the message to be more user friendly
  • Move throwing of error on the API level and the helper functions just returning 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
Copy link
Contributor Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants