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

[Code Review] 동구 / week9 #15

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

[Code Review] 동구 / week9 #15

wants to merge 1 commit into from

Conversation

dyk-im
Copy link
Contributor

@dyk-im dyk-im commented Dec 8, 2024

donggu 디렉터리가 아닌 week9-donggu 디렉터리가 코드리뷰 대상 입니다!

@dyk-im dyk-im added the documentation Improvements or additions to documentation label Dec 8, 2024
@dyk-im dyk-im self-assigned this Dec 8, 2024
@dyk-im dyk-im linked an issue Dec 8, 2024 that may be closed by this pull request
@dyk-im dyk-im changed the title [submit] : donggu / week9 [Code Review] : donggu / week9 Dec 8, 2024
@dyk-im dyk-im removed the documentation Improvements or additions to documentation label Dec 8, 2024
@dyk-im dyk-im changed the title [Code Review] : donggu / week9 [Code Review] 동구 / week9 Dec 8, 2024
@mmingoo mmingoo requested review from junseokkim and mmingoo December 9, 2024 08:41
Copy link

@mmingoo mmingoo left a comment

Choose a reason for hiding this comment

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

전반적으로 코드를 잘 작성해주셔서 제가 언급한 부분만 수정해주시면 더 좋은 코드가 될 거 같습니다. :)

@NotNull
@OneToOne(fetch = FetchType.LAZY)
@JoinColumn(name = "question_id")
private Question question;
Copy link

Choose a reason for hiding this comment

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

반적으로 컬렉션 필드는 다음과 같은 이유로 final로 선언하는 것이 권장됩니다.

  1. 불변성 방지
    추후에 코드를 작성할 때 의도치 않게 필드의 값이 변경되는 것을 막아줍니다.

  2. 초기화 강제
    필드에 fianl 을 붙이면 반드시 초기화를 해야 합니다.
    만약 final이 붙은 필드를 초기화하지 않을 시 컴파일 에러가 발생합니다.
    모든 필드에 final 을 붙이면 모든 필드가 초기화된 상태로 코드가 실행되기 떄문에 안정성이 향상됩니다.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

현재 파일에서는 컬렉션 필드가 보이지 않으나 코드 전체적으로 매핑을 위해 선언되어있는 컬렉션 필드 등과 관련하여 리뷰 주신 것으로 이해했습니다.
private final List answers = new ArrayList<>();
이런 식으로 final을 선언해 변경을 방지하고, 강제 초기화를 하도록 수정하는 것이 리뷰 주신대로 수정한 것이 맞나요?

Copy link

Choose a reason for hiding this comment

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

  1. 엔티티 선언엔 final을 붙이는 것을 추천하지 않고,
  2. 엔티티를 구성하는 필드 ex) name, title엔 final을 붙여주고
  3. 컬렉션과 리스트엔 붙여주는 걸 추천합니다.

엔티티 선언
public class Question { ... } // final 사용 X

기본 필드 선언

private final String title;    // final 사용 O
private final String content;  // final 사용 O

컬렉션과 리스트 선언

private final List<Comment> comments = new ArrayList<>();  // final 사용 O
private final Set<Tag> tags = new HashSet<>();           // final 사용 O

엔티티 선언

@ManyToOne
private User user;           // final 사용 X
@OneToOne
private Profile profile;     // final 사용 X

.build();
}

public void confirmQuestion(Answer answer) {
Copy link

Choose a reason for hiding this comment

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

불필요한 파라미터는 없애는 게 좋을 거 같습니다.

다음처럼 코드를 수정하면 좋을 거 같습니다.

public void confirmQuestion() {
    this.confirmStatus = true;
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

해당 파라미터의 경우 수정 과정에서 누락된 것 같습니다. 수정하도록 하겠습니다!

public ApiResponse<MissionResponseDto.addUserMissionResultDto> addMission(
@RequestBody @Valid MissionSelectRequestDto request,
@PathVariable Long missionId) {
UserMission userMission = missionService.addUserMission(request);
Copy link

Choose a reason for hiding this comment

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

해당 내용은 controller에 작성하는 것 보단 service 단에 작성하는 게 더 맞는 방법입니다.

컨트롤러는 HTTP 요청/응답 처리, 파라미터 검증, 응답 변환에 집중합니다.
UserMisson과 같은 도메인 객체가 controller 단에 노출되는 것은 다음과 같은 문제가 있습니다:

  1. 보안상 취약점 발생 가능성
  2. 도메인 객체의 변경이 API 스펙에 직접적인 영향을 미침
  3. 불필요한 정보가 클라이언트에 노출될 수 있음

Copy link
Contributor Author

Choose a reason for hiding this comment

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

addUserMission의 경우 Service레이어에 정의된 함수로 이관되어 처리되고 있습니다.
아래에서 피드백 주신대로 컨버터를 Service 레이어로 옮겨봤고

@PostMapping("/{missionId}/select")
	@Operation(summary = "유저 미션 도전", description = "특정 가게의 미션에 도전합니다.")
	public ApiResponse<MissionResponseDto.addUserMissionResultDto> addMission(
		@RequestBody @Valid MissionSelectRequestDto request,
		@PathVariable Long missionId) {
		return ApiResponse.onSuccess(missionService.addUserMission(request));
	}

onSuccess()의 인자 타입은 MissionResponseDto.addUserMissionResultDto로 dto를 활용했습니다.
이런식으로 코드를 수정하는 것이 리뷰 남겨주신 것과 동일한 방식일까요???

Copy link

Choose a reason for hiding this comment

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

넵 근데 확장성 혹은 추후에 수정하기 간편하게 다음처럼 코드를 변경하는 것을 추천합니다.

@PostMapping("/{missionId}/select")
@Operation(summary = "유저 미션 도전", description = "특정 가게의 미션에 도전합니다.")
public ApiResponse<MissionResponseDto.addUserMissionResultDto> addMission(
    @RequestBody @Valid MissionSelectRequestDto request,
    @PathVariable Long missionId) {
    
    MissionResponseDto.addUserMissionResultDto missionResultDto = missionService.addUserMission(request);
    return ApiResponse.onSuccess(missionResultDto);
}

@RequestBody @Valid MissionSelectRequestDto request,
@PathVariable Long missionId) {
UserMission userMission = missionService.addUserMission(request);
return ApiResponse.onSuccess(MissionConverter.toUserMissionResponseDto(userMission));
Copy link

Choose a reason for hiding this comment

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

DTO 변환을 service 계층에서 처리하는 것을 권장합니다. controller에서 DTO 변환을 진행할 경우 장단점이 존재합니다.

장점

  1. 서비스 계층의 응집도 향상(유지보수 향상)
  2. 컨트롤러가 더 간단해짐
  3. 도메인 객체 노출 방지

단점:

  1. 서비스 계층이 표현 계층(DTO)에 대한 의존성을 가지게 됨
  2. 동일한 도메인 객체에 대해 다른 DTO 변환이 필요한 경우 서비스 메소드를 추가해야 할 수 있음

Copy link
Contributor Author

Choose a reason for hiding this comment

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

해당 코드의 경우 실습과 미션을 진행하며 같은 방식으로 작성한 코드입니다. 리뷰 주신대로 장단점이 명확하다고 생각하는데 혹시 어떤 방식을 선호하시는 지 궁금합니다..!

Copy link

Choose a reason for hiding this comment

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

바로 위에 다시 답글 달아놨습니다 :)

public ApiResponse<MissionResponseDto.myMissionPreviewListDto> getMyOngoingMission(
@PathVariable Long userId,
@CheckPage @RequestParam(name="page") Integer page) {
int convertedPage = (page > 0) ? page - 1 : 0;
Copy link

Choose a reason for hiding this comment

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

page를 계산할 때 현재 코드는 page가 0보다 큰지 아닌지에 대한 로직이 들어가있습니다.
해당 코드보다는 제가 제안하는 코드를 사용하면 조건 로직이 빠져서 코드가 간결해질 거 같습니다.

// 현재
@RequestParam(name="page") Integer page
int convertedPage = (page > 0) ? page - 1 : 0;

// 제안
@RequestParam(name="page", defaultValue = "1") int page
int convertedPage = page - 1;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

이 부분의 경우 page 값 검증 전에 잘못된 값을 처리하고자 비교 로직을 만든 것인데 불필요한 코드가 맞는 것 같습니다! 의견 감사합니다

@Entity
@Getter
@NoArgsConstructor(access = AccessLevel.PROTECTED)
public class UserPrefer extends BaseTimeEntity {
Copy link

Choose a reason for hiding this comment

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

  1. 테이블명
    테이블명은 보통 스네이크 케이스(snake_case)를 사용합니다.

  2. id
    기본키는 보통 단순히 'id'로 명명합니다.
    테이블명이 이미 해당 엔티티를 나타내므로, 'userPreferId'와 같이 반복적인 이름은 불필요합니다.

  3. 코드 수정 예시

@Entity
@Table(name = "user_prefer")
@Getter
@NoArgsConstructor(access = AccessLevel.PROTECTED)
public class UserPrefer extends BaseTimeEntity {

    @Id
    @GeneratedValue(strategy = GenerationType.IDENTITY)
    private Long id;

    @NotNull
    @ManyToOne(fetch = FetchType.LAZY)
    @JoinColumn(name = "user_id")
    private User user;

    @NotNull
    @ManyToOne(fetch = FetchType.LAZY)
    @JoinColumn(name = "food_category_id")  // 이것도 스네이크 케이스로
    private FoodCategory foodCategory;
~~ 나머지 코드 ~~
}

위 코드와 같이 코드를 작성하면

  1. 더 간결하고 일관된 코드 작성이 가능하고
  2. 다른 개발자들과의 협업이 용이합니다.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

아래 리뷰까지 참조하여 테이블 명과 id 필드 명 수정하도록 하겠습니다. id 필드의 경우도 다른 주차 피드백 해주셨었는데 제가 반영하지 못했던 것 같습니다...

Copy link

Choose a reason for hiding this comment

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

오.. 제 피드백을 봐주셨다니.. 감사합니다 ㅎㅎ


@NotNull
@ManyToOne(fetch = FetchType.LAZY)
@JoinColumn(name = "food_id")
Copy link

Choose a reason for hiding this comment

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

  1. JoinColum명 변경
@JoinColumn(name = "food_id")
private FoodCategory foodCategory;

위 코드를 이것도 스네이크 케이스 기법으로 하여 다음처럼 변경해줍니다.

@JoinColumn(name = "food_category_id")

@dyk-im
Copy link
Contributor Author

dyk-im commented Dec 15, 2024

정성스런 리뷰 감사합니다! 참조해서 수정하고 궁금한 점 답글로 남겼습니다!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[feat] 가게 리뷰 목록 조회 API-week9
2 participants