8조 리뷰입니다 - 강기주 튜터

우선 8조원 모두 너무 고생하셨습니다. 배달 서비스가 난이도가 높음에도 선택하신 부분 너무 멋있습니다 :)

커스텀 에러를 order, review, user 를 만들어주신 부분 너무 좋습니다!

아래 코드를 보면 Controller 를 생성할 때 usersService, pointRepository, usersRepository 를 사용있습니다. 3계층을 분리하였기 때문데 Controller -> Repository 를 직접 접근은 하지 않도록 지켜주시는 것이 좋습니다! 설령 Service 의 로직이 없고 단순 repository 를 사용하는 한 줄이더라도 분리하는게 좋습니다! 이유는 추후 비지니스 로직이 추가되었을 때 Controller 과 Repository 코드 변경을 최소화할 수 있기 때문입니다.

const usersController = new UsersController(usersService, pointRepository, usersRepository);

아래 코드는 user.controller 의 userSignUp 코드 중 일부입니다. package.json 을 보니 joi 라이브러리가 추가되었던데 joi 라이브러리를 사용해서 개선해보시면 좋을 것 같습니다.

또한 == 비교를 사용하고 계신데 === 를 사용하시는게 좋습니다. A == B 의 경우 A 와 B 가 다를 경우 타입을 암시적 변환을 통해 비교해서 원치 않는 동작을 할 수 있기에 === 사용하셔서 타입 비교까지 해주시는게 좋습니다!

      const { email, name, password, confirmPassword, addr, number, role } = req.body;
      const emailPattern = /^[A-Za-z0-9_\.\-]+@[A-Za-z0-9\-]+\.[A-za-z0-9\-]+/;
      const numberPattern = /^[0-9]{2,3}-[0-9]{3,4}-[0-9]{4}/;
      if (!email || !name || !password || !confirmPassword || !addr || !number || !role)
        return res.status(400).json({ message: '필수 값을 입력하지 않았습니다.' });
      if (password !== confirmPassword)
        return res.status(400).json({ message: '비밀번호와 비밀번호 확인이 일치하지 않습니다.' });
      if (!['user', 'owner'].includes(role)) return res.status(400).json({ message: '올바르지 않은 권한입니다.' });
      if (emailPattern.test(email) == false)
        return res.status(400).json({ message: '이메일 형식이 올바르지 않습니다.' });
      if (numberPattern.test(number) == false)
        return res.status(400).json({ message: '전화번호 형식이 올바르지 않습니다.' });

변수는 소문자로 시작해야합니다!

const Menu = await this.menusRepository.getMenu(storeId, menuId);
    if (!Menu) throw new Error('존재 하지 않는 메뉴 입니다.');

    const MenuName = await this.menusRepository.getMenuName(storeId, menuName);
    if (MenuName) throw new Error('이미 중복된 메뉴 이름이 있습니다.');

아래 코드를 보면 status 유무에 따라서 분기처리를 해주었는데 코드가 중복적입니다. 이 부분도 개선해보시면 좋을 것 같습니다.

if (!status) {
      orders = await this.prisma.orders.findMany({
        where: { storeId: store.storeId },
        orderBy: {
          createdAt: value,
        },
        take: +perPage,
        skip: (page - 1) * perPage,
      });
    } else {
      orders = await this.prisma.orders.findMany({
        where: { storeId: store.storeId, status: status },
        orderBy: {
          createdAt: value,
        },
        take: +perPage,
        skip: (page - 1) * perPage,
      });
    }

OrdersRepository order 함수를 보면 길게 코드가 적혀있는데 비지니스 로직이 담겨있습니다! 해당 코드에서 서비스로 분리하도록 해주시면 좋을 것 같네요!

아래 코드중 userId 변수를 보면 userId.money 와 같이 사용하고 있습니다. 변수명은 userId 에 15124 와 같은 값이 들어갈 것이라고 생각하는데 변수명을 고민해보세요! 또한 사용자가 가입하고 포인트를 사용하고 /sendEmail 를 호출하면 포인트가 다시 적립될 것으로 보이는데 한 번 확인해주세요!

     const userId = await this.pointRepository.findUserById(user.userId);
      const validationCode = generateValidationCode(6);
      if (user.role !== 'user') return res.status(400).json({ message: '사용자만 포인트 적립을 받을 수 있습니다.' });
      if (userId.money === 1000000) return res.status(400).json({ message: '이미 포인트를 지급하였습니다.' });

user.service 의 generateToken 함수를 보면 단순히 토큰을 발행하는데 util 로 분리하지 않고 service 에 있는게 적합한지 고민해주세요!

generateToken = async (email) => {
    const token = jwt.sign({ email }, process.env.SECRET_KEY, { expiresIn: '1h' });
    return token;
  };

에외 처리를 다양하게 해주신 반면 테스트 코드에서는 예외 처리를 확인하고 있지 않네요! 테스트 케이스에서도 예외 처리가 정상적으로 이루어지는지 확인이 이루어지면 너무 좋을 것 같습니다.

order.service.spec 을 보면 테스트 케이스 이름이 orderCheckById, orderCheck 와 같이 작성되어있는데 좀 더 명확히 서술해줄 필요가 있습니다.

테스트 케이스를 보면 controller, service, repository 만 존재하는데 유닛 테스트로 jwt token, validation 등 여러 함수들에 테스트 케이스도 있으면 너무 좋을 것 같습니다.

제가 8조에 가서 이야기를 나누었던 부분 참고하셔서 개선해보시면 좋을 것 같습니다! 너무 고생하셨고 필요하신 부분 있으시면 언제든지 찾아와주세요!

Uhan33 / hungry-web

8조 리뷰입니다 - 강기주 튜터 #17