Skip to content
This repository has been archived by the owner on Aug 11, 2021. It is now read-only.

Misc Code Review Concerns #1

Open
hemangandhi opened this issue Jul 10, 2020 · 0 comments
Open

Misc Code Review Concerns #1

hemangandhi opened this issue Jul 10, 2020 · 0 comments

Comments

@hemangandhi
Copy link

Sorry this is a couple days late. The angry github tab pointing at my procrastination finally got to me.

Here are a couple initial concerns about the code (with heavy assumptions on what the end result is expected to be since this project really needs a readme).

  1. Please add a README. If you do, I'm not sure who the best reader is to have the right mix of prior knowledge and cluelessness about this project. Also consider a CONTRIBUTING.md so that you can have docs catering to contributors more.
    1. I think, optimally, there'd be a repo-level readme explaining the user stories (or an approximation thereof) and extra READMEs for the backend and frontend folders that focus more on the technical details of the two. CONTRIBUTING.md's in the folders might make sense, but could be over-complicated too -- it depends on what you expect a contributor to look like.
  2. This use of dicts will lead to a key error if the ID passed in does not exist. Consider having a default response for that case -- this will be a lot more user-friendly (even if the user here is the frontend) than a 5** HTTP error.
  3. How many servers does this lead to? It seems that every *_service.py file is a distinct server. Why?
    1. Is there a real scalability concern if you just kept these are separate paths served by the same server?
    2. If you want two separate servers, are they going to run on the same machine? If so, I'd recommend making the ports configurable. Even if they will be on separate machines in production, having the port be a flag will significantly ease testing.
  4. (I think you're already critical of this, so this is just a suggestion of an alternative.) Instead of using a modal, what if you replaced it with a div whose visibility may be toggled?

Let me know if you'd prefer splitting these into different issues or any other means of discussion.

(Why do I sound like an adult? 😱 )

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

No branches or pull requests

1 participant