Skip to content
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

Feature-#26 #28

Merged
merged 6 commits into from
Jul 16, 2024
Merged

Feature-#26 #28

merged 6 commits into from
Jul 16, 2024

Conversation

KoungQ
Copy link
Member

@KoungQ KoungQ commented Jul 15, 2024

1. 개발내용

  1. 가입된 동아리원 조회 기능 추가
  2. dto class -> record
  3. mapper 스프링 빈 등록
  4. exception 구체화
  5. 스웨거 summary 등록

2. 구현 세부사항

멤버들을 전체 조회하지만 기수별로도 조회하므로 리스트를 정렬만 된 상태로 통으로 넘겨주는게 오버헤드가 더 적을지, 아니면 여기서 groupingBy해서 넘겨주는게 오버헤드가 더 적을지 고민이었습니다.
후자를 선택했을 때 데이터 자체는 중복되므로 response가 무거워지긴 하지만 어차피 페이지에 들어오고 새로고침을 하지 않는 이상 한 번만 요청을 하므로, 프론트에서 기수를 선택할 때마다 전체 데이터에서 필터링을 하기보다 프론트에서 연산을 생략할 수 있도록 주는 족족 바로 가져다 쓸 수 있는 상태가 더 오버헤드가 적을 것 같다고 생각했습니다.
그리고 Map<Integer, List<UserDTO.Response>>와 페이지네이션 중 어떤 식으로 리턴해주는게 더 나을까 생각했지만 가능한 API 요청의 수를 줄일 수 있으면 줄이는 게 낫다고 판단하여 RequestParameter로 ~~~?page=1 와 같이 파라미터로 여러번 요청해야 한다는 생각에 Map으로 전달했습니다.
Map의 key값은 기수, value는 해당 기수의 인원들을 담아두었습니다.

3. 메모

@KoungQ KoungQ added Feature 기능 개발 Refactor 코드 리펙토링 labels Jul 15, 2024
@KoungQ KoungQ self-assigned this Jul 15, 2024
@KoungQ KoungQ changed the title Feature #26 Feature-#26 Jul 15, 2024
@KoungQ KoungQ linked an issue Jul 15, 2024 that may be closed by this pull request
))
.collect(Collectors.groupingBy(Map.Entry::getKey, // key = 기수, value = 유저 정보
Collectors.mapping(Map.Entry::getValue, Collectors.toList())));
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

모든 엔티티를 가져올 때 저는 findAll()만 썼었는데 참고하겠습니다!


user.applyOB(cardinal);
}

public Map<Integer, List<UserDTO.Response>> findUsers() {
return userRepository.findAll().stream()
.filter(user -> user.getStatus() == ACTIVE)
Copy link
Member

@jiixon jiixon Jul 15, 2024

Choose a reason for hiding this comment

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

쿼리로 먼저 ACTIVE한 유저를 가져 온 후 구현하는것이 불필요한 메모리를 줄일 수 있지 않을까요?! stream에서 필터로 구현하신 이유가 있으신지 궁금합니다!

Copy link
Member Author

@KoungQ KoungQ Jul 16, 2024

Choose a reason for hiding this comment

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

오 ACTIVE한 유저만 넣자는 생각을 마지막에 해서 그 생각을 못했네요. (완료)
감사합니다!

Copy link
Member

@jiixon jiixon left a comment

Choose a reason for hiding this comment

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

제가 생각해도 클라이언트에서는 데이터를 그룹핑하거나 필터링할 필요가 없이 코드를 단순화 하는 것이 더 나은 방법이라고 생각합니다!! response의 크기는 .. 예로 메인페이지에서 많은 양의 데이터를 줘야한다면, 그 부분을 캐쉬메모리에 저장하는 등 호출의 최소화를 하는 것도 하나의 방법으로 알고 있어요,

하지만 데이터의 규모가 그렇게 크지 않을 것 같다고 생각합니다! UX를 고려하여 프론트의 코드 연산 최소화가 더 좋다고 생각해요. 로직상 문제가 없는 것 같습니다! 리뷰 확인하시고 머지하셔도 좋을 것 같아요:)

그리고 close #이슈번호 하면 이슈 닫혀욤~ 안 쓰셨길래 쓰시면 편하답니당

Comment on lines +5 to +9
public class UserNotFoundException extends EntityNotFoundException {
public UserNotFoundException() {
super("존재하지 않는 유저입니다.");
}
}
Copy link
Member

Choose a reason for hiding this comment

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

이전 PR 에서도 UserNotFoundException()이 종종 보였는데 이전 PR 코드도 전부 고쳐진 건가요??

Copy link
Member Author

Choose a reason for hiding this comment

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

다른 분들 도메인에서는 제대로 수정이 이뤄지고 있을진 모르겠지만
저번 회의에선 통일하자는 얘기 했고 PR 올리기 전에 이 포맷으로 일치시키자는 소통은 된 상황입니다.
다른 분들의 코드는 다음 회의 시간 전으로 올라오는 PR 확인해보고 포맷 통일 시킬 수 있도록 힘 써보겠습니다.
감사합니다!

@NotNull private Department department;
@NotNull private Integer cardinal;
}
public record SignUp (
Copy link
Member

Choose a reason for hiding this comment

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

record 사용 좋습니다:)~~!!

Copy link
Member

@hyxklee hyxklee left a comment

Choose a reason for hiding this comment

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

고생하셨습니다!

@@ -9,28 +10,40 @@
import lombok.RequiredArgsConstructor;
import org.springframework.web.bind.annotation.*;

import java.util.List;
import java.util.Map;

Copy link
Member

Choose a reason for hiding this comment

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

@tag(name = "", description = "") 어노테이션 사용해서 해당 컨트롤러에 대한 설명도 추가할 수 있습니다!

Copy link
Member Author

@KoungQ KoungQ Jul 16, 2024

Choose a reason for hiding this comment

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

추가하겠습니다. (완료)
감사합니다!

@KoungQ KoungQ merged commit ee1ca96 into main Jul 16, 2024
1 check passed
@KoungQ KoungQ deleted the feature-#26 branch July 16, 2024 04:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature 기능 개발 Refactor 코드 리펙토링
Projects
None yet
Development

Successfully merging this pull request may close these issues.

#26 Feat: 가입된 동아리 원 조회 기능 추가
4 participants