Skip to content

Conversation

@yut-code
Copy link

@yut-code yut-code commented Jun 17, 2025

Notion ticket link

Matchmaking endpoints

Implementation description

  • used pet and user service methods as much as possible without hindering performance.
  • implemented matchmaking service + methods + routes

Steps to test

  1. Change pet color levels and check <= or >= for findMatchingPets and findMatchingUsers
  2. Check what happens when id in slug doesn't exist
Screenshot 2025-06-16 at 9 29 58 PM one user can take care of pet Screenshot 2025-06-16 at 9 29 42 PM no users can take care of pet Screenshot 2025-06-16 at 9 31 00 PM pets user can take care of Screenshot 2025-06-16 at 9 31 34 PM

no user found. similar message for pet

What should reviewers focus on?

  • If getting all users is not efficient enough (interacting with ORM vs just using the service methods)
  • Not much else I can think of

Checklist

  • My PR name is descriptive and in imperative tense
  • My commit messages are descriptive and in imperative tense. My commits are atomic and trivial commits are squashed or fixup'd into non-trivial commits
  • I have run the appropriate linter(s)
  • I have requested a review from the PL, as well as other devs who have background knowledge on this PR or who will be building on top of this PR

@yut-code yut-code marked this pull request as ready for review July 3, 2025 23:37
@sthuray sthuray self-requested a review July 4, 2025 20:00
Copy link
Collaborator

@sthuray sthuray left a comment

Choose a reason for hiding this comment

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

We've kinda separated matchmaking services / functions unnecessarily

To make it a bit more intuitive, I'd suggest restructuring it like this:

Change endpoints:

  • /matchmaking/users/:userId/pets ----> /users/match/:userId
  • /matchmaking/pets/:petId/users ----> /pets/match/:petId
  • get rid of /matchmaking router

Change services:

  • matchmakingService.getMatchingUsersForPet(petId)
    ----> userService.getMatchingPetsForUser(userId)
  • matchmakingService.getMatchingPetsForUser(userId) ----> petService.getMatchingUsersForPet(petId)
  • get rid of matchmakingService

@sthuray sthuray assigned sthuray and yut-code and unassigned sthuray Jul 4, 2025
@yut-code yut-code requested a review from sthuray July 18, 2025 01:39
const { userId } = req.params;

if (userId) {
if (typeof userId !== "string") {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Creating a middleware to handle the param validation will make this code a lot neater

See /middlewares/validators for examples of how other endpoints used it
-> create a matchPetsValidator function in petValidators

Do a similar process for /match/users/:petId

@yut-code yut-code requested a review from sthuray July 28, 2025 21:29
@sthuray sthuray requested a review from Artyom-G September 29, 2025 23:32
Copy link
Member

@Artyom-G Artyom-G left a comment

Choose a reason for hiding this comment

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

image

The route is a bit awkward imo, same thing for the pets/match/pets.

Also think the methods should be documented more as to what they do, maybe a short descriptor at the function header. Other than that it works as it should

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.

3 participants