-
Notifications
You must be signed in to change notification settings - Fork 0
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
[Feat] 소셜 링크 등록 API (온보딩) #47
Conversation
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.
고생했어요
User user = SecurityUtil.getUser(); | ||
List<Link> links = requestDto.getLinks().stream() | ||
.map(linkDto -> Link.of(linkDto.getUrl(), linkDto.getType())) | ||
.collect(Collectors.toList()); | ||
|
||
profileService.createSocialLinks(requestDto.getProfileId(), user.getId(), links); | ||
return ApiResponse.ok(); |
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.
서비스 layer에 fit한 dto를 주고싶으면 만드는 bean을 생성하는게 좋지않을까요
import java.util.List; | ||
|
||
@Data | ||
public class SocialLinkCreateRequestDto { |
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.
nit : dto이름이 넘 길어요 request는빼도될듯
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.
여기는 response dto가 없지만 다른 Dto들은 ~~ReqeustDto, ~~ResponseDto 이렇게 돼있어서요. 여기서 더 줄이려면 Request, Response까지만 적고 Dto를 빼는건 어떨까요?
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.
좋아요 다른 Dto도 다바꿔야겠군요 ㅋ
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.
이 PR에서 한번 다 갈아버리고 갈게요 ㅋㅋㅋ
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.
아 근데 service layer 전용 dto가 나오면 ... 애매해지는데 ㅎ 일단 홀딩
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.
서비스 레이어 dto를 api모듈의 서비스보다는 domain 모듈의 서비스에 맞춰지는게 좋다고 생각해서요. 그런데 이걸 윗부분에 적용해보려니 애매해지네요...
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.
dto 가공하는걸 도메인서비스에서쓰든 api서비스에서 쓰든 네이밍을 어캐할거냐는 고민은 그대로긴하죠 ㅎ
api서비스에서 request Dto를 유지하는거면.. api서비스 계층 이후에서 LinkDto -> Link작업, SecurityUtil로 User꺼내는 작업을 하든 해야되지않을까요
api서비스에선 그냥 requestdto만 파라미터로 받아도 될거같아여
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.
api의 서비스에 request dto를 바로 넘기는 방법도 좋은 것 같네요.
그렇게 한다면 클라이언트와의 요청, 응답을 ~Request, ~Response로 하고, 도메인 서비스의 dto를 ~Dto로 하는건 어떨까요?
그러면 여기서는 SocialLinkCreateRequest, SocialLinkCreateResponse, SocialLinkCreateDto 이렇게 나올 것 같아요
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.
예예 그렇게가시죠
throw new KeeweException(KeeweRtnConsts.ERR424); | ||
} | ||
|
||
if(!urlObj.getHost().equals(type.getDomain())) { |
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.
getHost()가 null일 가능성없나요?
getDomain은 값이 있음이 보장되므로..
if(!urlObj.getHost().equals(type.getDomain())) { | |
if(!type.getDomain().equals(urlObj.getHost())) { |
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.
getHost()가 빈 문자열을 반환하는거로 보이긴 하지만 확신하질 못하겠네요,, 제안해주신대로 바꾸는게 더 명확할 것 같아요
#32