abumostafa_gh-service_feedback

feedback.md

Overall it's a small repo so my comments are not very serious issues. It'd be great to see a larger project that you worked on. Also, send me your CV so I can have more context on your experience

You probably already know some points in the feedback, but I don't want to assume that so I'm sending all my comments

Good points

• nice project structure
• good use of async/await

Improvements

• In a serious project, you probably don't want to write your own date validation regex. Your time is better spent on business logic. moment & date-fns are two popular libraries for this

• Same for handling request timeout, axios might be a good option here

• File https://github.com/abumostafa/gh-service/blob/master/src/service/github.js

  • no param validaiton other than date, joi is a popular lib for validation
  • const q avoid short vague variable names, maybe githubFilterParam?
  • q.map(decodeURIComponent) should this be encode or decode?
  • lang && typeof lang === "string" why do we need lang &&
  • page && !isNaN(Number(page)) why do we need page &&
  • date && typeof date === "string" && isValidDate(date) why not move all these checks to the validator?
  • const url = `${GITHUB_API_HOST}/search/repositories?${queryStr}`; better to use a URL builder library than string concatenation to avoid extra bugs due to / or ?. axios will also take care of this
  • Try to avoid creating objects then mutating them later like query & q. It makes it harder to debug and understand the code as you might need several logs/breakpoints before and after every change to see what happened. Can be rewritten like

  const repoFiltersFromParams = [
    ...isValidDate(date) ? [`created:>${date}`] : [],
    ...typeof lang === "string" ? [`language:${lang}`] : [],
  ];
  
  const repoFilters = repoFiltersFromParams.length ? repoFiltersFromParams : ["stars:>0"]
  
  const query = new URLSearchParams({
    sort: "stars",
    order: "desc",
    page: isNaN(Number(page))? 1: page,
    itemsPerPage: isNaN(Number(itemsPerPage)) ? 10 : itemsPerPage,
    q: repoFilters.join('+'), // calling query.toString() should take care of encoding
  });

• why use supervisor with docker? better to have docker handle restarts so you can monitor with docker tools

• real prod app will use something cluster or pm2 to run more than 1 process

• docker-compose should not have volumes for src in prod

• better keep test files next to source files to make finding them easier

• File https://github.com/abumostafa/gh-service/blob/master/test/functional/service/github.test.js

  • Test does not validate the function return. It's fine in this case, but in a real app probably need to test it

• File https://github.com/abumostafa/gh-service/blob/master/test/intergration/controller/repo.test.js

  • describe/test callbacks can be replaced with arrow functions
  • You can make test names more expressive by nesting describe blocks. Easier to read and when a test fails it will give you more context

  describe("Repo Controller", () => {
  
    describe("When home is called", () => {
      it("should return ok", async () => {
          const res = await request(app).get("/");
          expect(res.status).toEqual(200);
      });
    });
  
    describe("When called with a wrong url", () => {
      it("should return 404", async () => {
        const res = await request(app).get("/v1/repo/search");
        expect(res.status).toEqual(404);
        expect(res.body).toEqual({ error: "Page Not Found" });
      });
    });
  
  ...