Feature/#49 내 명함에서 활동을 추가 삭제하는 기능 #52
Conversation
- 나중에 Delete에서 DTO Request를 구분하기 위함 #49
|
|
||
| public static List<MemberCareerResponse> from(final List<MemberCareer> memberCareers) { | ||
| final EnumMap<ActivityType, List<Career>> groupByActivityType = | ||
| groupingByActivityTypeAndSortedByCareerName(memberCareers); |
| } | ||
| } | ||
|
|
||
| public List<MemberCareerResponse> addCareer( |
There was a problem hiding this comment.
이건 개인적인 코드 취향입니다!!
public 메서드들 사이에 private 메서드가 껴있는게 저는 조금 어색하게 느껴집니다.
특히, saveMemberCareers 메서드의 경우 registerCareer에서도 사용되지만 addCareer에서도 사용되고 있는데 registerCareer 밑에, addCarrer 위에 위치해서 더욱 어색하게 느껴지는 것 같아요.
There was a problem hiding this comment.
이 부분은 진짜 취향인 것 같네요 .저는 굳이 public private 기준으로 분류할 필요는 없다고 생각하거든요.
위에서 아래로 읽었을 때 바로 인지할 수 있으면 된다고 생각해요.
코드 배치정도까지는 각자 편한 스타일로 작성하는건 어떨까요?
| @RequiredArgsConstructor | ||
| public class MemberCareerResponse { | ||
|
|
||
| private final String activityName; |
There was a problem hiding this comment.
여기 activityName과 아래 MemberActivityResponse의 name 필드만 봐선 도메인 지식이 없으면 헷갈릴 것 같아요. 나중에 재정의 할 필요가 있어 보입니다.
There was a problem hiding this comment.
말씀해주신 것처럼 activity career라는 용어는 인지하기 어려울 것 같아요. 심지어 이번에 직무도 빠졌으니까 조금 더 애매한 용어가 되버린 것 같네요. 고유명사에 대한 논의를 한 번 해보고 크게 리팩터링 해봐야할 것 같네요.
| @Test | ||
| @DisplayName("사용자 정보를 잘 저장하면, 204 no Content를 반환해준다.") | ||
| @DisplayName("사용자 정보를 잘 저장하면, 204 no Content를 반환해줄 수 있다.") |
There was a problem hiding this comment.
혹시 "반환해준다"에서 "반환해줄 수 있다"로 변경하셨는데 이유가 있으신가요?
There was a problem hiding this comment.
다른 테스트 코드와 displayName을 맞추기 위해서에요. 큰 의미는 없습니다. 저랑 우르랑 번갈아가면서 하다가 싱크가 안 맞았던 부분인 것 같아요.
| @@ -87,3 +89,33 @@ include::{snippets}/wish-with-no-authentication-products/request-headers.adoc[] | |||
|
|
|||
| .HTTP response | |||
| include::{snippets}/wish-with-no-authentication-products/http-response.adoc[] | |||
|
|
|||
| == Career | |||
There was a problem hiding this comment.
NIT : '커리어'라는 이름을 봤을 때 정확히 어떤 기능이 포함될지 예상하기 어려운 것 같아요(직관성이 떨어지는 것 같습니다)
API 명세는 읽기 위한 문서이니 저희 팀의 고유 명사를 확실히 정해서 조금 더 이해하기 쉽게 서술하면 좋을 것 같아요!
There was a problem hiding this comment.
고유명사를 다같이 논의를 해봐야할 것 같네요. 월요일에 논의해봐요!
| :source-highlighter: highlightjs | ||
| :toc: left | ||
| :toclevels: 2 | ||
| :sectlinks: | ||
|
|
||
| == Product |
There was a problem hiding this comment.
Product는 이전 기획과 관련된 명세이니 지워도 괜찮을 것 같습니다!
There was a problem hiding this comment.
RestDocs를 위한 예시 코드였어요. 삭제하도록 할게요!
| .collect(toList()); | ||
| } | ||
|
|
||
| private static EnumMap<ActivityType, List<Career>> groupingByActivityTypeAndSortedByCareerName( |
There was a problem hiding this comment.
from() 메서드에서 mapToMemberActivityResponses()보다 groupingByActivityTypeAndSortedByCareerName()가 먼저 사용되었으니 두 메서드의 순서를 바꿔주는 편이 가독성 측면에서 더 좋을 것 같아요!
로직은 흠잡을 데 없는 것 같습니다👍👍
There was a problem hiding this comment.
앗 이 피드백을 미쳐 못보고 머지해버렸네요. 추가로 하나 PR 파서 빠르게 수정하고 머지하도록 하겠습니다.
| @Test | ||
| @DisplayName("사용자 정보를 잘 저장하면, 204 no Content를 반환해준다.") | ||
| @DisplayName("사용자 정보를 잘 저장하면, 204 no Content를 반환해줄 수 있다.") |
There was a problem hiding this comment.
NIT: 나중에 '사용자 정보', '활동 이력' 등의 도메인 용어 사전(한/영문)을 다같이 정리해보면 좋을 것 같습니다. 이건 제가 기억했다가 다음에 다같이 있을 때 다시 얘기 꺼내볼게요!
There was a problem hiding this comment.
이 용어 관련해서는 다같이 이야기를 나눠봐요. 지금은 확실히 용어가 어색한 면이 있네요.
| List.of( | ||
| new MemberActivityResponse(1L, "YAPP"), | ||
| new MemberActivityResponse(2L, "DND"), | ||
| new MemberActivityResponse(3L, "nexters") |
There was a problem hiding this comment.
NIT: 이런 데이터는 추후 Fixture로 분리해도 좋을 것 같습니다!
There was a problem hiding this comment.
넵. 추후 이러한 객체들이 더 많이 쓰이면 분리하는 방향을 모색해봐요
#️⃣연관된 이슈
📝작업 내용
💬리뷰 요구사항
close #49