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

hector upload code example for review, function alter at /:exerciseId… #3

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

hector918
Copy link
Collaborator

…/submissions/:submissionId/run

req.general_wraper = async (req, res, fn, error_callback) => {
//if this coding style got Accepted, it should be put at middleware, put it here for coding exmaple
//
req.user.id = (await prisma.user.findUnique({ where: { firebaseId: req.user.uid } })).id;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Think this should be good, non-authenticated view shouldn't be a priority right now so this should be helpful

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I going to move it to the app.js, and add a cache for it, building this for structure use in near future, I going to put authorization everywhere i touched

include: {
files: true
//I going to wrap the "try catch" to an general function for future debug and logging demand
req.general_wraper = async (req, res, fn, error_callback) => {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typo in wrapper though

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

corrected, and changed to camel case

@RF9A5V
Copy link
Collaborator

RF9A5V commented Jan 12, 2024

Good start, but a few things:

  1. Would be great if you could keep with the same coding conventions we've got. You're using snake case similar to what you'd see in C code, but to stay consistent with what's in the project, please use camel case.
  2. I don't believe we need the additional fields that represent whether a fellow has directly completed the developing or proficient exercises. I believe that if the level field on the UserTopic is already set to Proficient, then we should already know that they have more than 5 exercises in Developing done, and if they're set to Advanced, then we should be able to infer that they've got more than 5 exercises in Proficient done. Happy to hear your thoughts on this though.
  3. Wrapper looks good for loading in the user if available, but potentially call it something a bit more self explanatory, like loadFirebaseUser or something. We may want the error handling in a separate piece of middleware.

@hector918
Copy link
Collaborator Author

1: snake case -> camel case changed
2: that is on the requirements, I removed it, and I think I can have my judgment in the future beside the requirement?
3: I trying to abstract the error handling and log and performance timer into the wrapper, nothing is once for all, but I am trying to,

@RF9A5V
Copy link
Collaborator

RF9A5V commented Jan 14, 2024

  1. Awesome, looks good
  2. Yes, generated the ticket with ChatGPT, hadn't gone over it well enough it looks like, but definitely you can exercise your own judgement and leave your work in the PR for review. Feel free to reach out and ask if you're considering different approaches as well
  3. We can roll out measuring metrics in a separate ticket, for now it'd be good to keep the work on this ticket solely related to the related work

@RF9A5V
Copy link
Collaborator

RF9A5V commented Jan 14, 2024

A few more issues to review

@hector918
Copy link
Collaborator Author

hector918 commented Jan 14, 2024

  1. We can roll out measuring metrics in a separate ticket, for now it'd be good to keep the work on this ticket solely related to the related work

now the code is a little merge together, and I properly need those function in the future work, how about merge it this time, I going to follow the one ticket one function one PR next time?

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

Successfully merging this pull request may close these issues.

2 participants