-
Notifications
You must be signed in to change notification settings - Fork 58
[백재우] sprint3 #149
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
The head ref may contain hidden characters: "part1-\uBC31\uC7AC\uC6B0-sprint3"
[백재우] sprint3 #149
Conversation
유저 이름,닉네임, 이메일 비즈니스 로직인 유효성 검증
생각해 볼 부분, 현재 static 으로 정적메서드가 많이 존재하는데, 검증해야하는 부분들이 많아질 경우 어떻게 할 것인가? 장단점 생각해보기 검증하는 로직을 외부 객체로 진행하게 된다면, 장단점 생각해보기
…print-mission into part1-백재우-sprint3
인터페이스 메소드 구현체 구현추가
checkAlready -> throwAlready
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
재우님 과제하시느라 고생하셨어요.!!
UUID channelId, | ||
String channelName, | ||
LocalDateTime lastMessageTime, | ||
Set<UUID> participatedUserId |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s 빠졌어요!
public JCFChannelService( | ||
ChannelRepository channelRepository, | ||
UserService userService, | ||
ReadStatusRepository readStatusService, | ||
MessageRepository messageRepository | ||
) { | ||
this.channelRepository = channelRepository; | ||
this.userService = userService; | ||
this.readStatusRepository = readStatusService; | ||
this.messageRepository = messageRepository; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@requiredargsconstructor 를 활용해보면 어떨까요 ?
|
||
@Override | ||
public void changeSubject(UUID userId, ChangeChannelSubjectRequestDto requestDto) { | ||
User foundUser = userService.findOneByUserIdOrThrow(userId); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
참조자료 보고 학습해보겠습니다!
@Override | ||
public void changeSubject(UUID userId, ChangeChannelSubjectRequestDto requestDto) { | ||
User foundUser = userService.findOneByUserIdOrThrow(userId); | ||
Channel foundChannel = findOneByChannelIdOrThrow(requestDto.channelId()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
여기도 마찬가지입니다.!
public UUID getId() { | ||
return id; | ||
} | ||
|
||
public Username getUsername() { | ||
return username; | ||
} | ||
|
||
public Email getEmail() { | ||
return email; | ||
} | ||
|
||
public String getUsernameValue() { | ||
return this.username.getValue(); | ||
} | ||
|
||
public String getNicknameValue() { | ||
return this.nickname.getValue(); | ||
} | ||
|
||
public String getEmailValue() { | ||
return this.email.getValue(); | ||
} | ||
|
||
public String getPasswordValue() { | ||
return this.password.getValue(); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
getter 를 활용하여 보일러 코드를 줄여보는건 어떨까요 ?
private void throwIsNotManager(User foundUser, Channel foundChannel) { | ||
if (!foundChannel.isManager(foundUser)) { | ||
throw new NotChannelManagerException(ErrorCode.CHANNEL_ADMIN_REQUIRED, foundUser.getUsernameValue()); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
throwIfNotManager 가 더 적합해보여요 ㅎㅎ
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이렇게 함수 만들어 서비스에서 에러를 던지는게 아니고 호출한쪽에서
channel.throwIfNotManager(user);
이렇게 만들어서 엔티티에서 에러를 던지는것에 대해서는 혹시 어떻게 생각하세요 ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
응집도가 높아지겠군요... 만약 수정한다면, 서비스 메서드 내부에서 함수를 기존 코드처럼 분리하지않고
객체의 메서드를 호출해서 검증할 수 있을 것 같은데요. 메서드 네이밍을 매니저가 아닌것을 확인하는게 메서드의 목적이니까 checkIsManager
로 생각해보았는데 어떤가요..?
// ====> 삼항 연산자와 if 문 분기처리 중 어떤 것을 더 선호해야할까요? 제가 생각하기에는 삼항 연산자가 보기 더 편한것 같습니다. | ||
// 다음 미션에서는 둘 중 하나 지워두겠습니다. | ||
// createChannel = requestDto.visibility() == ChannelVisibility.PUBLIC | ||
// ? Channel.ofPublicChannel(requestDto.name(), requestDto.channelType(), foundUser) | ||
// : Channel.ofPrivateChannel(foundUser, requestDto.channelType()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
지금 상황에서는 if-else 가 더 명시적이며, 나중에 enum 에 값이 추가된다고 했을때도 유리해보여요./
안녕하세요 멘토님! 👋
2주차 이후 코드를 보다가 도메인 로직과 비즈니스 로직이 혼합되어 분리가 어려워져 처음부터 다시 코딩했습니다.
어느정도 틀은 잡았지만 다음과 같은 문제가 있었습니다.
파일 관련 코드의 이해 부족 => 제공된 베이스 코드에서는 각 도메인 객체의 id 값으로 파일이름을 생성하여 관리
dto에 파일을 담는방법 => 감이 안잡혔습니다.
최대한 멘토님이 리뷰해주신 내용을 적용하여 의미를 담도록 네이밍하고 복잡하지 않도록 노력하였습니다.
멘토에게
질문
예를 들어서 게시글을 저장하는 로직이 존재한다고 가정하겠습니다.
Controller에서는 파라미터로 요청 DTO 객체를 받아서 Service레이어로 넘겨 처리합니다. 그리고 서비스에서는 컨트롤러에서 값을 받아 비즈니스 로직을 수행합니다.
이 때 서비스 레이어서는 DTO의 객체를 알 필요는 없다고 생각하기도하고, 알아도 딱히 문제가 된다고 생각하지 않습니다.
또한, DTO 클래스 안에 객체와의 변환을 담당하는 메서드를 만들어 DTO 역할을 확장하여 서비스 레이어 코드가 간결해지기도 합니다. (개인적으로 DTO에 도메인 객체를 노출시키는 것에 대해선 부자연스럽다고 생각하지만, 서치한 결과와 사람들이 많이 사용하는 방식인 것 같습니다..)
같은 기능을 Converter 객체를 만들어서 toDto, toEntity 등 변환을 담당하는 서비스 객체를 만들어 DTO와 도메인 객체 간의 결합도를 낮추어 활용하는 방법도 있습니다. 이 방법은 도메인이 늘어날 경우 관리해야하는 코드의 수가 늘어나는 단점이 존재하기는 합니다.
다음과 같이 DTO 안에서 변환을 담당하는 것과 converter 객체를 만들어서 변환하는 것에 대해서 멘토님은 어떻게 생각하고 계시는지 궁금합니다. 그리고 컨트롤러에서 DTO 객체를 적절한 엔티티 클래로 변환하여 서비스에서는 파라미터 매개변수에는 일관적으로 DTO 클래스를 받지 않도록 하는 것에 대해서 어떻게 생각하느시는지 알려주세요 😄
제가 조사해보니 정답은 없는 것 같습니다. 다만
SOLID
원칙과 유지보수성을 고려해서 결정해야 한다는 결론을 내릴 수 있었지만, 경험 부족으로 결정하기 쉽지 않다는 것입니다. 하나의 컨트롤러에서 여러개의 서비스 이용할 경우를 고려하면 서비스 레이어에서는 도메인 객체를 리턴하는게 보다 유연할 것 같기도 합니다.그리고 현재 코드가 하나의 서비스에서 여러 서비스를 의존하고 있는데, 연관관계가 있는 도메인의 서비스들이 많아질 수록 복잡해질 것 같다고 생각합니다.