[Code Review] 준재 code review #18

jjun522 · 2024-12-15T02:31:06Z

No description provided.

junseokkim · 2024-12-15T09:32:18Z

src/main/java/umc/spring/web/controller/StoreController.java

+    private final StoreQueryServiceImpl storeQueryService;
+
+
+    @PostMapping("{regionId}")


해당 부분에서 앞에 /를 생략하여도 자동으로 경로를 처리하기 때문에 아마 동작은 잘 됬을 거라고 생각되네요. 다만, 코드 일관성을 위해서 /를 붙여주는 것이 더 좋아 보입니다!!! 이러면 더 명확하게 URL 경로임을 보여줄 수 있을 거 같네요.

Suggested change

@PostMapping("{regionId}")

@PostMapping("/{regionId}")

junseokkim · 2024-12-15T09:37:10Z

src/main/java/umc/spring/web/controller/StoreController.java

+        StoreResponseDTO.JoinResultDTO result = storeQueryService.joinStore(regionId, request);
+        return ApiResponse.onSuccess(storeQueryService.joinStore(regionId, request));


현재 storeQueryService.joinStore(regionId, request)를 두번 호출하고 있네요!!! 중복 호출은 성능 저하나 오류를 유발할 수도 있으니 다음과 같이 수정하면 좋을 거 같아요!!!

Suggested change

StoreResponseDTO.JoinResultDTO result = storeQueryService.joinStore(regionId, request);

return ApiResponse.onSuccess(storeQueryService.joinStore(regionId, request));

StoreResponseDTO.JoinResultDTO result = storeQueryService.joinStore(regionId, request);

return ApiResponse.onSuccess(result));

junseokkim · 2024-12-15T09:40:01Z

src/main/java/umc/spring/web/controller/StoreController.java

+    @PostMapping("{regionId}")
+    public ApiResponse<StoreResponseDTO.JoinResultDTO>


아래 특정 가게 리뷰 목록 조회 API처럼 Swagger를 통해 문서화를 추가해주면 더 좋을 것 같아요!!

Suggested change

@PostMapping("{regionId}")

public ApiResponse<StoreResponseDTO.JoinResultDTO>

@PostMapping("{regionId}")

@Operation(summary = "가게 등록 API", description = "지역 ID와 가게 정보를 받아 새로운 가게를 등록합니다.")

@ApiResponses({

@ApiResponse(responseCode = "200", description = "가게 등록 성공"),

})

public ApiResponse<StoreResponseDTO.JoinResultDTO>

junseokkim · 2024-12-15T09:41:37Z

src/main/java/umc/spring/web/controller/StoreController.java

+    public ApiResponse<StoreResponseDTO.JoinResultDTO>
+    join(
+            @PathVariable Long regionId,
+            @RequestBody @Valid StoreRequestDTO.joinStoreDTO request
+    ) {


코드 일관성을 위해 줄 바꿈 형태를 통일하는게 좋을 거 같아요!!

Suggested change

public ApiResponse<StoreResponseDTO.JoinResultDTO>

join(

@PathVariable Long regionId,

@RequestBody @Valid StoreRequestDTO.joinStoreDTO request

) {

public ApiResponse<StoreResponseDTO.JoinResultDTO> join(

@PathVariable Long regionId,

@RequestBody @Valid StoreRequestDTO.joinStoreDTO request

) {

junseokkim · 2024-12-15T09:42:29Z

src/main/java/umc/spring/web/controller/StoreController.java

+    @Operation(
+            summary = "특정 가게의 리뷰 목록 조회 API",
+            description = "특정 가게의 리뷰들을 조회하는 API로, 페이징 처리 기능을 제공합니다. query string으로 page 번호를 전달해야 합니다."
+    )
+    @ApiResponses({
+            @io.swagger.v3.oas.annotations.responses.ApiResponse(responseCode = "COMMON200", description = "OK, 성공"),
+            @io.swagger.v3.oas.annotations.responses.ApiResponse(responseCode = "AUTH003", description = "access 토큰을 주세요!", content = @Content(schema = @Schema(implementation = ApiResponse.class))),
+            @io.swagger.v3.oas.annotations.responses.ApiResponse(responseCode = "AUTH004", description = "access 토큰 만료", content = @Content(schema = @Schema(implementation = ApiResponse.class))),
+            @io.swagger.v3.oas.annotations.responses.ApiResponse(responseCode = "AUTH006", description = "access 토큰 모양이 이상함", content = @Content(schema = @Schema(implementation = ApiResponse.class)))
+    })


API 문서화를 너무 잘 하신거 같네요!!! 아마 협업 하실 때 프론트 개발자 분들이 스웨거 참고해서 할 텐데 이렇게 자세하게 문서화해주면 매우 효율적일 거 같아요

junseokkim · 2024-12-15T09:44:47Z

src/main/java/umc/spring/web/controller/StoreController.java

+    @ApiResponses({
+            @io.swagger.v3.oas.annotations.responses.ApiResponse(responseCode = "COMMON200", description = "OK, 성공"),
+            @io.swagger.v3.oas.annotations.responses.ApiResponse(responseCode = "AUTH003", description = "access 토큰을 주세요!", content = @Content(schema = @Schema(implementation = ApiResponse.class))),
+            @io.swagger.v3.oas.annotations.responses.ApiResponse(responseCode = "AUTH004", description = "access 토큰 만료", content = @Content(schema = @Schema(implementation = ApiResponse.class))),
+            @io.swagger.v3.oas.annotations.responses.ApiResponse(responseCode = "AUTH006", description = "access 토큰 모양이 이상함", content = @Content(schema = @Schema(implementation = ApiResponse.class)))
+    })


다만, AUTH 같은 공통적인 응답의 경우 별도로 파일로 구분해서 보여주는 것이 코드가 더 간결할 거 같아요!!!

public class CommonApiResponses { public static final @ApiResponse responseCode200 = @ApiResponse(responseCode = "COMMON200", description = "OK, 성공"); public static final ApiResponse AUTH003 = @ApiResponse( responseCode = "AUTH003", description = "access 토큰을 주세요!", content = @Content(schema = @Schema(implementation = ApiResponse.class)) ); }

이렇게 공통 응답을 정의하고,

@ApiResponses({ CommonApiResponses.COMMON200, CommonApiResponses.AUTH003, })

이런 식으로 참조하도록 한다면, 코드가 훨씬 간결해질 수 있을 거 같아요!!!

junseokkim · 2024-12-15T09:53:01Z

src/main/java/umc/spring/web/controller/MissionController.java

+    private final MissionQueryService missionService;
+
+    @PostMapping("/add")
+    public ApiResponse<MissionResponseDTO.JoinResultDTO> addMission(@RequestBody @Valid MissionRequestDTO.JoinAddMissionDTO request){


한 줄의 길이가 가능한 줄여서, 코드 가독성을 높여주는 것도 중요한 거 같아요!!! 나중에 프로젝트하실 때 팀 컨벤션에 맞춰서 코드를 작성하면 좋을 거 같네요!!

Suggested change

public ApiResponse<MissionResponseDTO.JoinResultDTO> addMission(@RequestBody @Valid MissionRequestDTO.JoinAddMissionDTO request){

public ApiResponse<MissionResponseDTO.JoinResultDTO> addMission(

@RequestBody @Valid MissionRequestDTO.JoinAddMissionDTO request

){

junseokkim · 2024-12-15T09:59:41Z

src/main/java/umc/spring/web/controller/MissionController.java

+
+    private final MissionQueryService missionService;
+
+    @PostMapping("/add")


RESTful 원칙에 따르면, 엔드포인트는 리소스 중심으로 설계해야 해요!! 따라서 mission을 추가하는 API 이므로 /missions와 POST 메서드의 의미를 통해 나타내는 것이 더 좋을 거 같아요

junseokkim · 2024-12-15T10:01:45Z

src/main/java/umc/spring/web/controller/MissionController.java

+@RestController
+@RequiredArgsConstructor
+@Validated
+@RequestMapping("/mission")


RESTful 설계에서 엔드포인트에서 사용되는 명사의 경우 복사형으로 사용하는 것이 더 좋아요!!

junseokkim · 2024-12-15T10:03:30Z

src/main/java/umc/spring/web/controller/MissionController.java

+    }
+
+
+    @GetMapping("/{storeId}/missions")


해당 엔드포인트의 경우에 missions가 중복으로 들어가네요!!! 다음과 같이 바꾸면 더 좋을 거 같아요

@GetMapping("/{storeId}")

junseokkim · 2024-12-15T10:07:27Z

src/main/java/umc/spring/web/controller/MyReviewController.java

+    private final ReviewQueryService reviewService;
+
+    @GetMapping("/{userId}/reviews")
+    @Operation(summary = "내가 작성한 리뷰 목록 조회 API", description = "특정 유저가 작성한 리뷰들의 목록을 페이징 처리하여 반환합니다.")


summary와 description의 내용이 일치하지 않아, 혼란을 줄 수 있을 거 같아요!!

만약, 내가 작성한 리뷰 목록 조회라면, 엔드포인트를 /users/me/reviews로 하는 것이 이후 특정 유저 리뷰 목록 조회와 겹치지 않을 거 같네요

junseokkim · 2024-12-15T10:08:09Z

src/main/java/umc/spring/web/controller/ReviewController.java

+
+@RestController
+@RequiredArgsConstructor
+@RequestMapping("/review")


이 부분 역시 명사형 대신에 복수형을 사용해주는 게 RESTful 설계 원칙에 따를 수 있을 거 같네요!!

junseokkim · 2024-12-15T10:55:19Z

src/main/java/umc/spring/converter/MissionChallengeConverter.java

+                    .build();
+        }
+
+        public static UserMission ToUserMission(MissionChallengeRequestDTO.JoinAddMissionChallengeDTO result, User user, Mission mission) {


result 변수는 함수 내에서 사용되지 않기 때문에 해당 파라미터는 제거해도 좋을 거 같네요

junseokkim · 2024-12-15T10:56:36Z

src/main/java/umc/spring/converter/MissionChallengeConverter.java

+            return MissionChallengeResponseDTO.JoinChallengeResultDTO.builder()
+                    .userMissionId(userMission.getId()) // UserMission의 ID
+                    .missionStatus(userMission.getMissionStatus().ordinal())
+                    .createdAt(LocalDateTime.now())


현재 코드에서 createdAt 필드가 LocalDateTime.now()로 설정되고 있습니다. 그러나 createdAt은 userMission이 생성된 정확한 시점을 나타내야 하므로, 변환 시점의 현재 시간이 아니라, 엔티티에 저장된 값을 그대로 사용하는 것이 더 적절합니다.

예를 들어, userMission이 데이터베이스에 저장되면서 createdAt 필드가 이미 설정되었을 것입니다. 이 값을 변환 과정에서 그대로 전달하면, 실제 생성된 시점의 정보를 유지할 수 있습니다. 반면, LocalDateTime.now()를 사용하면 변환 시점의 시간으로 덮어쓰게 되어, 데이터의 정확성과 무결성이 떨어질 수 있습니다.

따라서, StoreResponseDTO.JoinResultDTO를 생성할 때, store.getCreatedAt() 값을 그대로 사용하도록 수정하는 것을 권장합니다. 이렇게 하면 실제 생성 시점 정보를 유지할 수 있어 데이터의 신뢰성을 높일 수 있습니다.

junseokkim · 2024-12-15T10:58:03Z

src/main/java/umc/spring/domain/emums/MemberStatus.java

+package umc.spring.domain.emums;
+
+public enum MemberStatus {
+    Active, Inactive


보통 ENUM 상수는 전역 상수와 유사한 역할을 하기 때문에 대문자와 언더스코어(_)를 사용해서 표현하는 것이 일반적인 컨벤션입니다!!!

junseokkim · 2024-12-15T11:09:48Z

src/main/java/umc/spring/service/MissionService/MissionQueryServiceImpl.java

+    private final StoreRepository storeRepository;
+    private final UserRepository userRepository;
+    private final UserMissionRepository userMissionRepository;


현재 Mission 서비스에서 Store, User, UserMission 레포지토리를 직접 접근하고 있습니다.
이는 서비스 간 결합도를 높이고, 책임 분리 원칙(SRP)을 위반할 가능성이 있습니다.
Store와 User와 관련된 작업은 각각 StoreService와 UserService를 통해 수행하도록 변경하는 것을 권장드립니다!!
이렇게 하면 서비스 간 의존성을 줄이고, 코드의 유지보수성과 테스트 용이성이 향상될 수 있습니다!!!

junseokkim · 2024-12-15T11:13:19Z

src/main/java/umc/spring/service/MissionService/MissionQueryServiceImpl.java

+        Store store = storeRepository.findById(request.getStoreId())
+                .orElseThrow(()-> new MissionHandler(Mission_STORE_NOT_FOUND));


예시로 이 부분을 해당 미션 서비스에서 레포지토리에 접근하는 것이 아닌,

StoreQueryService에 다음과 같이

public Store getStore(Long storeId) { return storeRepository.findById(storeId()) .orElseThrow(()-> new MissionHandler(Mission_STORE_NOT_FOUND)); }

이렇게 함수를 만들어주고 mission 서비스에서는 이 store 서비스 함수를 호출하는 방식으로 하는 것이 좋습니다.

Suggested change

Store store = storeRepository.findById(request.getStoreId())

.orElseThrow(()-> new MissionHandler(Mission_STORE_NOT_FOUND));

Store store = storeQueryService.getstore(request.getStoreId());

junseokkim

우선, 미션 수행하시느라 정말 고생 많으셨습니다! 😊

대부분은 공통적이고 대중적인 관점에서 작성하려고 했지만, 일부 제 개인적인 의견이 포함되어 있을 수 있습니다. 따라서 코드 리뷰를 참고하시되, 무조건적으로 수용하기보다는 “왜 이렇게 해야 할까?”를 고민하고 찾아본 후 본인의 코드 스타일에 맞게 적용하는 것을 추천드립니다!

아래는 코멘트를 달앗지만 공통적으로 수정이 필요한 부분을 작성했습니다!!!

RESTful 엔드포인트 설계
- 현재 엔드포인트 설계가 전반적으로 잘 되어 있지만, 일부 경로에 동사(예: /add)가 포함되어 RESTful 설계 원칙과 맞지 않는 부분이 있습니다.
- RESTful 설계 원칙을 공부해보시고, 엔드포인트를 명사형으로 표현하도록 개선해보세요!
Swagger 문서화
- 일부 API에만 Swagger 문서화가 진행되어 있는데, 전체 API에 문서화를 추가한다면 협업 및 API 유지보수에 큰 도움이 될 것입니다.
코드 스타일
- 코드 한 줄의 길이가 너무 긴 경우가 있습니다. 줄바꿈을 통해 가독성을 개선하고, 메서드 선언부나 Builder 패턴 등에서 구조를 명확히 표현하면 더욱 좋습니다.
DTO 설계
- DTO에서 @Setter를 사용하는 대신, 불변 객체로 설계하는 것을 추천드립니다.
  @AllArgsConstructor와 final을 사용하면 객체의 안정성과 가독성을 높일 수 있습니다.
- 또한, DTO 멤버 변수에는 private 접근 제한자를 사용하여 캡슐화를 보장하는 것이 좋습니다.
createdAt 필드 처리
- 엔티티 생성 시 createdAt 필드를 LocalDateTime.now()로 설정하고 있는데, 이는 DB에 저장되면서 자동으로 설정된 값과 다를 수 있습니다.
  DB에 저장된 실제 createdAt 값을 그대로 사용하도록 수정하는 것을 권장드립니다.
서비스와 레포지토리 간의 책임 분리
- 현재 Mission 서비스에서 여러 레포지토리(Store, User, UserMission)에 직접 접근하고 있어 결합도가 높아지고 중복 코드가 발생하고 있습니다.
- 한 서비스에서 여러 레포지토리에 접근하기보다, 각 도메인의 로직은 해당 도메인 서비스(StoreService, UserService 등)에서 처리하도록 분리해주세요.
  이를 통해 코드 중복을 줄이고, **책임 분리 원칙(SRP)**을 준수할 수 있습니다.
  다만, 서비스 간 호출 시 순환 참조가 발생하지 않도록 의존성 방향을 잘 설계해야 합니다.

혹시나 코드리뷰에 대해서 궁금한 점이 있다면 답변 달아주시면 확인해볼게요!! 미션 수행하시느라 정말 고생 많으셨습니다! 🎉

jjun522 added 9 commits October 31, 2024 13:00

feat: 전체 커밋

62ddd50

feat: 전체 커밋

11da9ae

fix: remove application.yml

a787806

Add week6

793a89f

Add week7

d0c0fec

feat: 특정 지역에 가게 추가하기 API 가게에 리뷰 추가하기 API 추가

52c39b8

week 8 추가

7dcdf1c

feat: Implement week 9 API. Fixes #5

5b997cf

Merge branch 'main' into feature/week9

3271da9

junseokkim reviewed Dec 15, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Code Review] 준재 code review #18

[Code Review] 준재 code review #18

jjun522 commented Dec 15, 2024

junseokkim Dec 15, 2024

junseokkim Dec 15, 2024

junseokkim Dec 15, 2024

junseokkim Dec 15, 2024

junseokkim Dec 15, 2024

junseokkim Dec 15, 2024

junseokkim Dec 15, 2024

junseokkim Dec 15, 2024

junseokkim Dec 15, 2024

junseokkim Dec 15, 2024

junseokkim Dec 15, 2024

junseokkim Dec 15, 2024

junseokkim Dec 15, 2024

junseokkim Dec 15, 2024

junseokkim Dec 15, 2024

junseokkim Dec 15, 2024

junseokkim Dec 15, 2024

junseokkim left a comment •

edited

Loading

		private final StoreQueryServiceImpl storeQueryService;


		@PostMapping("{regionId}")

		StoreResponseDTO.JoinResultDTO result = storeQueryService.joinStore(regionId, request);
		return ApiResponse.onSuccess(storeQueryService.joinStore(regionId, request));

		@PostMapping("{regionId}")
		public ApiResponse<StoreResponseDTO.JoinResultDTO>


		private final MissionQueryService missionService;

		@PostMapping("/add")

		Store store = storeRepository.findById(request.getStoreId())
		.orElseThrow(()-> new MissionHandler(Mission_STORE_NOT_FOUND));

	Store store = storeRepository.findById(request.getStoreId())
	.orElseThrow(()-> new MissionHandler(Mission_STORE_NOT_FOUND));
	Store store = storeQueryService.getstore(request.getStoreId());

[Code Review] 준재 code review #18

Are you sure you want to change the base?

[Code Review] 준재 code review #18

Conversation

jjun522 commented Dec 15, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

junseokkim left a comment • edited Loading

Choose a reason for hiding this comment

junseokkim left a comment •

edited

Loading