Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

✨ [Feature/room] implement create room #30

Merged
merged 17 commits into from
Feb 20, 2025
Merged

Conversation

yunyoung1819
Copy link
Collaborator

@yunyoung1819 yunyoung1819 commented Feb 18, 2025

#️⃣ 연관된 이슈

📝 작업 내용

  • Added Room domain model and factory method
  • Added CreateRoomUseCase with persistence adapter.
  • Added RoomController to handle room creation request

📸 스크린샷 (선택)

💬 리뷰 요구사항(선택)

- Added Room domain model and factory method
- Added CreateRoomUseCase with persistence adapter.
- Added RoomController to handle room creation request
@yunyoung1819 yunyoung1819 added the ✨ Feature new feature label Feb 18, 2025
@yunyoung1819 yunyoung1819 added this to the ⭐ sprint-2 milestone Feb 18, 2025
@yunyoung1819 yunyoung1819 self-assigned this Feb 18, 2025
@yunyoung1819 yunyoung1819 changed the title ✨ feat: implement room create feature ✨ [feature/room] implement room create feature Feb 18, 2025
@yunyoung1819 yunyoung1819 changed the title ✨ [feature/room] implement room create feature ✨ [Feature/room] implement create room Feb 18, 2025
@yunyoung1819 yunyoung1819 marked this pull request as ready for review February 18, 2025 10:36
Copy link
Member

@linirini linirini left a comment

Choose a reason for hiding this comment

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

리뷰가 늦어서 죄송합니다 ㅜㅜ
코드 보면서 많이 배워갑니다 👍👍
몇 가지 궁금한 점과 저의 의견을 남겨보았습니다 ㅎㅎ
화이팅~!~!

Copy link
Member

Choose a reason for hiding this comment

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

EOL이 누락되었네용!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

음 mac이랑 window로 달라서 EOL 차이가 발생하는 것 같아요.
저는 git config --global core.autocrlf가 이미 input으로 설정되어 있어서 변경하지 않아도 된다고 하는데
혹시 git config --global core.autocrlf 가 어떻게 설정되어 있으실까여? true가 아니면 true로 변경해보시겠어요? 😄

Copy link
Member

Choose a reason for hiding this comment

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

image

github에서 마지막에 개행이 없으면 이와 같이 보여주고 있어요! 이 부분을 의미한거였습니다 ㅎㅎ

import lombok.Data;
import lombok.NoArgsConstructor;

@Data
Copy link
Member

Choose a reason for hiding this comment

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

record를 사용하지 않고 @Data를 사용한 이유가 궁금해요~

Copy link
Collaborator Author

@yunyoung1819 yunyoung1819 Feb 20, 2025

Choose a reason for hiding this comment

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

record나 class 둘 다 구현 가능한데 팀원들마다 record나 class로 각자 구현하는 방식이 다를 수 있다보니 통일성을 위해서 class로 구현하였고, getter, setter, toString 등의 반복 코드를 줄이기 위해 @Data를 사용했습니다. request나 response의 dto에 대해서는 다같이 record로 구현하는 식으로 맞춰보아도 좋을 것 같습니다 👍

@Configuration
public class UseCaseConfig {
@Bean
public CreateRoomUseCase createRoomUseCase(RoomRepository roomRepository) {
Copy link
Member

Choose a reason for hiding this comment

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

createRoomUsecase 인터페이스를 구현한 RoomService는 @Service로 빈으로 등록해주고 있는데, 추가적으로 수동 등록한 이유가 궁금해요!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

따로 수동 등록하지 않아도 되지만 인터페이스와 구현체 간의 의존성을 명시적으로 관리하면, 도메인 계층이나 유스케이스 계층이 구체적인 구현에 의존하지 않고 추상화된 인터페이스에만 의존하게 되어 의존성 역전 원칙(DIP)를 보다 명확하게 준수할 수 있는 장점이 있어서 위 방식을 한번 써보았습니다~ 😄

this.roomLinkUrl = roomLinkUrl;
}

public static Room create(String roomName, int capacity, String hostProfile, String password) {
Copy link
Member

Choose a reason for hiding this comment

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

생성자가 아닌 정적 팩터리 메서드로 분리한 이유가 궁금합니다!
VO를 분리하여 검증 책임을 적절히 위임한다면 주생성자 - 부생성자 형식으로 생성자 체이닝도 가능할 것 같아요!

Copy link
Collaborator Author

@yunyoung1819 yunyoung1819 Feb 20, 2025

Choose a reason for hiding this comment

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

주생성자 - 부생성자 형식으로도 구현해서 검증은 부생성자에 위임하는 방식으로 생성자 체이닝 방식도 좋지만, 정적 팩토리 메서드로 구현하는 것이 몇가지 더 장점이 있다고 생각했어요.

  1. 아래처럼 this()를 이용해서 주생성자를 호출하는 것보다는 메서드 이름을 통해 목적을 명확하게 표현가능
this(roomId, roomName, capacity, hostProfile, password, roomLinkUrl);
  1. 반환 타입의 하위 타입 객체를 반환도 가능
  2. this(roomName, password), this(roomName, password, hostProfile), ... 처럼 부생성자가 많아지는 경우 어떤 목적인지 명확히 알기 어렵고, 매개변수가 타입은 같은데 한두개 순서만 다른 경우 다른 사람이 잘못 사용할 수도 있음

이펙티브 자바에도 비슷한 내용이 나오는데 이외에도 장단점이 더 있지만 정적 팩터리 메서드가 좀더 낫다고 생각했습니다 😄

@@ -19,6 +19,8 @@ repositories {

dependencies {
implementation 'org.springframework.boot:spring-boot-starter'
implementation 'org.springframework.boot:spring-boot-starter-data-redis'
Copy link
Member

Choose a reason for hiding this comment

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

redis는 api 모듈에서 사용하는 것 같은데, core 모듈에 추가한 이유가 궁금해요~

Copy link
Collaborator Author

@yunyoung1819 yunyoung1819 Feb 20, 2025

Choose a reason for hiding this comment

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

저희가 도메인과 엔티티를 따로 분리하지 않고 같이 쓰기로 했었다보니 core 모듈에서도 redis 관련 애노테이션이 쓰일 수 있어 core 모듈에 추가했어요~

예를 들어 Room 객체를 Redis에 hash 타입으로 저장하는 것 등으로 바꾼다면

@RedisHash("room")
public class Room {
  ...
}

이렇게 쓰이게 되어 core 모듈에 추가했었습니다 😄

Copy link
Collaborator

@minseokey minseokey left a comment

Choose a reason for hiding this comment

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

LGTM! 코드 구조 많이 참고 하겠습니다~

Copy link
Member

@linirini linirini left a comment

Choose a reason for hiding this comment

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

리뷰 확인했습니다!
새롭게 추가된 유효성 검증에 대한 exception handler만 추가하면 될 것 같아서 approve 남깁니다:)
수고하셨어용~!~!

@yunyoung1819 yunyoung1819 merged commit d473748 into develop Feb 20, 2025
2 checks passed
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
✨ Feature new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants