Skip to content

Conversation

@KindNeighbor
Copy link
Contributor

@KindNeighbor KindNeighbor commented Feb 8, 2023

변경사항

📌 AS-IS

  1. kafka, websocket 설정 해 놓았습니다.
  2. 웹소켓 접속시 채팅 구현
  3. 채팅방 생성 / 삭제 구현
  4. Theater entity setter 추가 (채팅 방 생성시 공연장 id fk 로 저장하기 위해서)
  5. 채팅방과 공연장의 관계를 1:1 로 바꾸었습니다.

📌 TO-BE

  1. 토큰이 들어오는 경우 2차 개발에 들어가야 합니다.

잘 봐주었으면 하는 부분

  1. 일단 몇몇 패키지 만들어 놓았습니다. dto, controller, repository 등 이상 없는지 한 번 봐주시고
    수정할 것 있으면 전부 말해주세요.
  2. 채팅방 생성 로직을 한 번 봐주셨으면 합니다. 일단 공연장ID로 있는지 없는지 유무를 판단하고, 그 이후에 없다면 방을 만들고 그때의 공연장 id를 fk로 저장했습니다. 방이 있다면 그대로 return 했습니다. 뭔가 좀 더 깔끔하게 할 수도 있을 것 같은데 일단은 pr먼저 해봅니다.
  3. 채팅방 생성시 컨트롤러 부분에다가 PathVariable로 theater_id를 받았습니다. 처음에는 RequestParam으로 할까 하다가 값이 하나라서 그냥 @PathVariable로 넣었습니다. 어떤게 나은가요??
  4. 채팅방 삭제의 경우 방이 있는지 없는지 검증하는 편이 좋은가요??
  5. kafka 관련 파일들의 경우 Configuration 하나, Component 하나, Service 하나 있어서 그냥 한 패키지에 넣어버리고 config 패키지에 두었습니다. 혹시 다르게 나누고 싶은 의견이 있다면 알려주세요.
  6. sl4j를 써서 로그를 남긴 것들이 있는데, 지우는 편이 좋은가요??
  7. pr 템플릿이 없더라구요. pr 전 커밋으로 넣어놓을까요??

테스트

테스트는 로직 확정되면 바로 하겠습니다.

  • 테스트 코드
  • API 테스트

@junga970
Copy link
Contributor

junga970 commented Feb 8, 2023

채팅방 생성시 컨트롤러 부분에다가 PathVariable로 theater_id를 받았습니다. 처음에는 RequestParam으로 할까 하다가 값이 하나라서 그냥 @PathVariable로 넣었습니다. 어떤게 나은가요??

kafka 관련 파일들의 경우 Configuration 하나, Component 하나, Service 하나 있어서 그냥 한 패키지에 넣어버리고 config 패키지에 두었습니다. 혹시 다르게 나누고 싶은 의견이 있다면 알려주세요.

  • 2개 이상이면 패키지로 묶는게 깔끔하고 좋은 것 같아요.

pr 템플릿이 없더라구요. pr 전 커밋으로 넣어놓을까요??

  • 제가 깜빡했네요ㅜㅜ 부탁드리겠습니다!

@junga970 junga970 self-requested a review February 9, 2023 10:10
@KindNeighbor
Copy link
Contributor Author

변경 사항

  1. 로그 제외 했습니다.
  2. 방 생성 / 삭제 로직 리펙토링 진행했습니다.
  3. url 변경했습니다.
  4. 테스트 코드 작성했습니다.

@KindNeighbor KindNeighbor merged commit 0583b18 into develop Feb 9, 2023
@KindNeighbor KindNeighbor deleted the feature/chat branch February 11, 2023 16:28
KindNeighbor added a commit that referenced this pull request Apr 8, 2023
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