-
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 #75 멤버 리스트 반환 수정 및 멤버 상세 조회 추가 #75
The head ref may contain hidden characters: "feat/#72/\uBA64\uBC84-\uB9AC\uC2A4\uD2B8-\uBC18\uD658-\uC218\uC815-\uBC0F-\uBA64\uBC84-\uC0C1\uC138-\uC870\uD68C-\uCD94\uAC00"
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.
고생하셨어요!
제 생각을 적어봤으니 읽어봐주시면 감사하겠습니다
그리고 스웨거에 실제 데이터를 넣어서 멤버 목록 조회/ 상세 조회해서 실제로 반환되는 값을 스크린샷으로 부탁드릴게요!
public CommonResponse<Response> findUser(@RequestParam Long userId) { | ||
return CommonResponse.createSuccess( | ||
USER_DETAILS_SUCCESS.getMessage(), | ||
userUseCase.findUserDetails(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 Response findUserDetails(Long userId) { | ||
User user = userGetService.find(userId); | ||
if (user == null) { |
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.
userGetService.find()
메서드 내부에서 null 체킹을 해서 예외를 던져주고 있기 때문에 useCase 단에서 따로 체크는 하지 않아도 될 것 같아요!
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.
수고하셨습니다!
.flatMap(user -> Stream.concat( | ||
user.getCardinals().stream() | ||
.map(cardinal -> new AbstractMap.SimpleEntry<>(cardinal, mapper.toSummaryResponse(user))), // 기수별 Map | ||
Stream.of(new AbstractMap.SimpleEntry<>(0, mapper.toSummaryResponse(user))) // 모든 기수는 cardinal 0에 저장 |
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.
mapper.toSummaryResponse(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.
해당 로직은 기존에 구현되어있던 로직과 동일한 형태로 작성했는데, 수정하지 않은 이유는 mapper.toSummaryResponse가 동일한 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.
구글링 해보니 동일한 객체를 반환하는 메서드를 반복적으로 호출하는 것만으로도 오버헤드가 발생되네요
해당 내용도 수정하겠습니다
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 내용
멤버 리스트 조회 API
멤버 상세 조회 API
PR 세부사항
기존 API를 수정하여 멤버 리스트를 반환할 때 데이터 양을 축소하였습니다
SummaryResponse를 통해 userId, name, cardinals, department 만 반환하도록 수정했습니다
GET으로 축소된 멤버 리스트 반환하는 내용 구현하였고,
멤버 상세 조회 API 내용도 구현하였습니다
기존 dto에 있는 Response 내용을 참조하기 보다는
새로운 UserResponse를 생성하여 멤버 상세 조회 API시 반환하도록 하였습니다
관련 스크린샷
주의사항
로직은 이전에 adminResponse 축소할때와 동일하게 진행 하였으나,
UserAdminController 반환 리스트가 아닌 UserController의 동아리 멤버 전체조회(전체/기수별) api를 올바르게 수정했습니다.
기존 findAll메서드와 AdminResponse는 그대로 유지하기 위해서 새로운 findAllUser 메서드를 추가하여 구현했습니다
체크 리스트