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

refactor: 회의 진행 후 변경된 요구사항 반영 #74

Merged
merged 11 commits into from
Sep 25, 2024

Conversation

eom-tae-in
Copy link
Owner

@eom-tae-in eom-tae-in commented Sep 25, 2024

📄 Summary

  • hobby, style 페이징 조회 시 각각 id값을 포함하여 넘겨주도록 변경
  • 셀프소개 페이징 조회, 셀프 소개 필터링�페이징 조회 api를 하나로 통합
  • 테스트용 회원 로그인 기능 구현(로그인 인증 해제하는 방법 대신 이 방법으로 진행)

이후에 member 하위 패키지에 존재하는 hobby, style을 분리하는 작업을 진행할 예정입니다.

hobby, style의 api를 수정하면서 test fixture의 구조도 변경해보았습니다. 확인 후 리뷰 부탁드립니다 :)

close #73

Copy link
Collaborator

@devholic22 devholic22 left a comment

Choose a reason for hiding this comment

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

수고하셨습니다! 많이 수정해야 할 것은 아닌 것 같아 approve 먼저 드리겠습니다.

@@ -14,6 +15,7 @@

@RequiredArgsConstructor
@Service
@Transactional
Copy link
Collaborator

Choose a reason for hiding this comment

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

저희 그동안 Service를 가장 아래로 하고 그 위에 Transactional을 붙이기로 했던 것 같습니다 :)

Copy link
Owner Author

Choose a reason for hiding this comment

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

네 수정하겠습니다!

@Valid
@NotEmpty(message = "선호 지역을 하나 이상 입력해주세요.")
List<CityRequest> cityRequests
List<String> cities
Copy link
Collaborator

Choose a reason for hiding this comment

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

혹시 validator를 쓰지 않고 자체 검증만 하시게 된 이유가 있으실까요?

Copy link
Owner Author

Choose a reason for hiding this comment

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

필터 기능을 사용할 수도 사용하지 않을수도 있기 때문에 validator로 빈 값에 대한 테스트를 하게 되면 필터를 사용하지 않는 요청이 에러로 나가게 됩니다. 필터를 적용한 페이징 기능과 기존의 페이징 기능을 합쳐서 필터를 사용하지 않는 페이징 요청을 처리하기 위해서 dto에서 유효하지 않는 요청에 대해서만 빈 리스트를 넘겨주는 방식으로 구현했습니다.

Copy link
Collaborator

Choose a reason for hiding this comment

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

필요에 따라 쓸 수도 있고 쓰지 않을 수도 있기 때문이었군요 이해했습니다 :)

import java.util.List;

@SuppressWarnings("NonAsciiCharacters")
public class 취미_응답_픽스처 {
Copy link
Collaborator

Choose a reason for hiding this comment

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

앞으로도 이렇게 픽스처가 많아질 경우 내장 클래스 이용해서 만드는 게 좋을 것 같습니다!

);
}
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

공백 있습니다 :)

Copy link
Owner Author

Choose a reason for hiding this comment

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

제거하겠습니다

public ResponseEntity<Void> saveSelfIntro(@RequestBody @Valid final SelfIntroCreateRequest selfIntroCreateRequest,
@AuthMember final Long memberId) {
public ResponseEntity<Void> saveSelfIntro(
@RequestBody @Valid final SelfIntroCreateRequest selfIntroCreateRequest,
Copy link
Collaborator

Choose a reason for hiding this comment

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

인자를 내리는 규칙을 정하면 좋을 것 같습니다.
저는 그동안 이렇게 인자가 긴 것들이 많았을 때는 기존처럼 메서드명 옆에 인자 하나 붙이고 그 다음 인자부터 아래로 내렸는데, 혹시 태인님께서 의도하신 것은 앞으로 인자가 긴 것들이 많을 때는 바로 인자를 메서드명 아래에 한 줄씩 작성하시는 것을 의도하신 것일지 여쭤봐도 될까요?

Copy link
Owner Author

Choose a reason for hiding this comment

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

네 맞습니다! 컨트롤러 같은 경우는 매개변수가 3개가 되지 않아도 줄바꿈을 해야하는 경우가 있습니다.
이렇게 변수명이 길어질 경우 적절한 수준으로 줄이거나 줄바꿈을 사용하여 가독성 좋은 방식으로 진행하는 게 좋아보입니다!
현준님은 어떻게 생각하시나요?

Copy link
Collaborator

Choose a reason for hiding this comment

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

저도 줄바꿈으로 가독성을 높이는 게 좋을 것 같습니다. 다만 본 작업 말고도 여러 번 컨트롤러 메서드명 옆에 하나의 인자를 뒀던 경우가 많아서, 지금 진행한 것 처럼 인자 모두를 줄바꿈으로 해두는 게 필요할 듯 합니다 :)

Copy link
Owner Author

Choose a reason for hiding this comment

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

추후에 전반적인 리팩터링 작업을 진행하여 변경하면 좋을 것 같습니다!

@eom-tae-in eom-tae-in merged commit 308576b into develop Sep 25, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

회의 진행 후 변경이 된 요구사항을 반영한다
2 participants