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

전남대_11조_골라주마_5주차 #45

Closed
wants to merge 100 commits into from
Closed

전남대_11조_골라주마_5주차 #45

wants to merge 100 commits into from

Conversation

jinwooseok
Copy link
Contributor

@jinwooseok jinwooseok commented Oct 8, 2023

어떤 내용에 대한 PR인가요?

안녕하세욥! 11조 진우석입니다! 5주차 진행사항 pr입니다

5주차 작업 완료 내역

[FEAT] 무한스크롤 시 사용할 response 추가 #28
[FIX] 시큐리티 의존성 제거 #34
[Bug] worflow 삭제 #36
[Feat] 인가 처리 구현 #37
[FEAT] 투표 리스트 조회 기능 #38
[REFACTOR] comment 리팩토링1 #39
[Feat] 회원 가입 단위 테스트 작성 #41

** 5주차 과제를 제출할 때 문제가 생겨서 그 전의 커밋도 모두 올라가서 저번 주만 포함된 부분은 여기입니다!**

참고사항

  1. 대부분이 로그인 유저와 비로그인 유저를 구분해서 로직을 처리하기 때문에 spring security를 사용했을 때 제약이 생기기도 하고, 직접 구현해 보고 싶다는 생각이 들어 도전을 해보게 됐습니다!
  2. 프로젝트의 큰 부분을 차지하는 투표 리스트 조회 기능을 구현했습니다. 최신순은 no offset방식으로 구현을 했고, 인기순은 피드백 주신대로 다른 방식을 사용해 만들어 볼 예정입니다. 조회에 다양한 처리 로직이 있어 내용이 많은 편인데 분리를 해야할지 고민 중입니다..
  3. jwt 인증을 활용했습니다. 부족한 점이 있다면 말씀해주시면 감사하겠습니다.
  4. 피드백과 객체지향 생활체조 원칙을 참고하면서 개발하려고 노력 중 입니다.. 글로 볼 땐 하면 하지 싶었지만 쉬운 일이 아니었습니다! 더 발전하고 싶습니다!
  5. 회원 가입 부분의 서비스 로직 테스트 코드를 작성했습니다.

궁금한 점

  • 질문 1

    • repository 쿼리 작성하는데는 여러가지 방식이 있는걸로 알고 있는데 em.createQuery를 사용하면 entity뿐만 아니라 dto를 기준으로 데이터를 불러올 수 있다는 게시글을 봤습니다. dto는 entity하고는 모양이 다른 부분이 많기 때문에 dto에 바로 넣는 쿼리를 작성하면 xxEntity, yyEntity에 해당하는 정보를 2번 불러올 걸 join 1번에 해결할 수 있을 것 같았습니다. 보통은 entity를 repository의 리턴값으로 사용을 하는 것 같은데 이런 방식도 많이 사용하나요?

      저희 팀이 외래키를 사용하지 않기 때문에 관계가 연결되어 있지 않아서 하나의 entity를 부를 때 다른 entity도 부르지 못합니다!

  • 질문 2

    • 투표 조회의 api는 sort=”desc”&active=”active”&category=”doing” 이런 식으로 파라미터로 3개의 파라미터를 받습니다. 그런데 이렇게 하다보니 서비스로직의 덩치가 커지는 것 같아서 분리를 해야할지 궁금합니다.
  • 질문 3

    • 저희는 HandlerMethodArgumentResolver를 통해서 토큰 claim에서 사용자 id를 가져와 로그인 된 사용자인지 판별합니다

      위와 같은 방식으로 각 컨트롤러 단에서 해결해야 할 문제를 공통적으로 해결하고 있습니다.

      하지만 개발을 하다보니깐 서비스단에서 해당하는 id가 존재하는 것인지, 또는 해당하는 글 id가 존재하는 것인지 등 repository에 질의를 하여 해당 Id가 존재하는 것인지 validation 해야하는 부분이 공통적으로 발생하는 것을 확인했습니다.

      따라서 이 부분은 AOP를 통해 해결해볼까? 라는 생각을 했지만

      AOP를 사용했을 때의 장점은 코드의 중복이 줄어든다.

      단점은 코드의 흐름(처음 코드를 보는 사람이 코드를 이해하기 어렵다.) 와 같은 명확한 장단점이 존재하는 것 같습니다.

      해당 하는 글 id가 존재하는 것인지 판별하는 것도 비즈니스 로직으로 봐서 해당 서비스에서 처리를 해야하는 게 좋을지

      아니면 중복으로 발생하는 validation 하는 부분을 AOP로 처리해도 좋을 지 궁금합니다!

  • 질문 4

    • custom validation을 사용할때 custom exception을 throw 해도 좋은지 궁금합니다. (더 세분화된 메세지와 응답 코드를 전달하기 위해)
    • 예시)

image

  • 질문 5
    • 투표 종료시점이 지나면 별도의 요청없이 엔티티의 active 속성을 변경(투표마감)해야하는데 어떻게 구현해야하는지 궁금합니다.

kssumin and others added 30 commits September 26, 2023 22:46
성공 응답 메시지, 성공 시 응답 상태를 가지고 있음
생성시간, 수정시간을 인지하여 자동으로 저장
[FEAT] 프로젝트 공통 기능 생성
kssumin and others added 27 commits October 8, 2023 13:49
@jinwooseok jinwooseok added the Request Review 멘토님께 PR리뷰 요청 label Oct 8, 2023
@kssumin kssumin closed this Oct 9, 2023
Copy link

@lalwr lalwr left a comment

Choose a reason for hiding this comment

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

1주동안 고생하셨습니다 💯

코드 잘 구현해주셨는데 피드백 드린 내용 관련해서 관련있는 코드가 있다면 같이 수정 해보시면 좋을꺼 같습니다!

테스트도 잘 추가해주셨습니다~

작업해야 될 내용에는 TODO로 남겨 놓으면 관리에 좋을꺼 같습니다~!

public CreateVoteResponse createVote(CreateVoteRequest requestDto) {
boolean exit;
// 1. 투표 제목이 있는지 확인 후 예외처리
if (requestDto.getTitle() == null) {
Copy link

Choose a reason for hiding this comment

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

controller 에서 벨리데이션으로 검증할 수 있지 않을까요?
(3번까지)

// 4. 카테고리가 맞는지 확인
Category.findCategory(requestDto.getCategory());

System.out.println(requestDto);
Copy link

Choose a reason for hiding this comment

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

println 보다는 로그를 활용하는것이 좋습니다.

VoteEntity vote = voteJPARepository.save(requestDto.toEntity());
long voteId = vote.getId();
for (CreateVoteRequest.OptionDTO option : requestDto.getOptions()) {
optionJPARepository.save(option.toEntity(voteId));
Copy link

Choose a reason for hiding this comment

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

reqeust dto 는 요청에 대한 처리만 해야하는데
entity화 시키는 작업은 entity에서 처리 하거나 mapstruct 와 같은 라이브러리와 같은 부분을 활용하는것이 어떨까요?
클래스의 역할에 맞게 사용하는 것이 좋습니다.


private final VoteJPARepository voteJPARepository;
private final OptionJPARepository optionJPARepository;
// private final DesicionJPARepository desicionJPARepository;
Copy link

Choose a reason for hiding this comment

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

사용하지 않는 코드는 제거해주세요~
나중에 구현해야할 코드라면 코드는 나중에 추가하고 주석을 통해 적어두는것이 좋을꺼 같습니다.

private final OptionJPARepository optionJPARepository;
// private final DesicionJPARepository desicionJPARepository;

static int page = 0;
Copy link

Choose a reason for hiding this comment

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

고정 값인것으로 보이는데 공통으로 사용가능한 변수로도 보입니다.
여러곳에서 사용하게 된다면 controller 에서 Page 관련 Dto를 만들고 Service 쪽에 넘겨주거나
Page 관련 클래스에서 사용하는것을 어떨까요?

options.add(option1);
options.add(option2);

CreateVoteRequest request = new CreateVoteRequest(null, "total", "...", 60, options);
Copy link

Choose a reason for hiding this comment

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

null 값을 셋팅해야 하는 경우 null인 값을 뺸 생성자를 만들어서 관리하는것이 좋습니다.

boolean isOwner;

Long id = commentEntity.getUserId();
System.out.println(id);
Copy link

Choose a reason for hiding this comment

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

로그를 활용해보세요~

Copy link
Contributor Author

Choose a reason for hiding this comment

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

넵 알겠습니다!


Long id = commentEntity.getUserId();
System.out.println(id);
String username = "asdf"; // 데이터베이스에서 유저 닉네임 가져오기 위한 레포지토리가 들어갈 부분 - 미완성
Copy link

Choose a reason for hiding this comment

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

TODO 로 추가하면 커밋 진행시 진행해야 될 내용에 대해 인지할 수 있습니다.

@InjectMocks private SaveUserService saveUserService;

@Test
@DisplayName("회원가입 시 이메일 중복체크를 한다.")
Copy link

Choose a reason for hiding this comment

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

이메일 중복과 닉네임 중복은 회원가입시 무조건 진행되는 항목이니 테스트 1개로 묶어서 해도 좋지 않을까요?

Copy link
Contributor

Choose a reason for hiding this comment

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

테스트 당 한 가지 항목만 테스트를 하기 위해서 위와 같이 진행을 하였습니다!
별도의 테스트로 분리해서 각자 하나씩만 검증하도록 하였는데,

두 항목이 "같이"진행되는 것이라면 한 테스트에서 수행하는 방식이 더 올바른지 궁금합니다!

import org.mockito.junit.jupiter.MockitoExtension;

@ExtendWith(MockitoExtension.class)
class ValidEmailUseCaseTest {
Copy link

Choose a reason for hiding this comment

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

💯

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Request Review 멘토님께 PR리뷰 요청
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants