해커톤 작업 내역을 리팩토링 #94

zangsu · 2024-07-19T08:10:38Z

⚡️ 관련 이슈

close #93

📍주요 변경 사항

불필요한 도메인 삭제
- Member
- Extension
- Language
커스텀 예외 추가
DTO 에 검증 어노테이션 추가
테스트코드 작성

🎸기타

현재 DTO 에 대한 예외 처리가 MethodArgumentNotValidException 에 대한 처리로 구현되어 있습니다. CodeZapException으로 감싸야 할지 고민입니다 🤔

Co-authored-by: hoeseong123 <[email protected]>

jminkkk

코드에서 고민의 흔적이 많이 보입니다👍 수고하셨어요!
리뷰 확인해주시면 감사하겠습니다 🍀

+) 추가로 변경 사항에 잡히지 않는 swagger 클래스들(ex. SpringDocTemplateController)도 확인해주시면 좋을 것 같아요!
이번 pr부터 검증 로직이 들어갔는데, 실패 케이스에 대해서도 문서화를 진행해주어야 프론트엔드 개발자가 예외 메시지나 포맷을 확인하고 처리를 수월하게 할 수 있습니다

backend/build.gradle

backend/src/main/java/codezap/global/auditing/JpaAuditingConfiguration.java

backend/src/main/java/codezap/global/serializing/DateTimeFormatConfiguration.java

backend/src/main/java/codezap/template/controller/TemplateController.java

backend/src/main/java/codezap/template/domain/Snippet.java

jminkkk · 2024-07-19T08:57:02Z

backend/src/main/java/codezap/template/dto/request/CreateSnippetRequest.java

        String content,
+        @NotNull(message = "스니펫 순서가 null 입니다.")


순서는 0부터 시작인가요?
순서 검증이 필요할 것 같아요!

해당 부분은 공부를 조금 더 해보고 적용해야 할 것 같아 이슈로 열어놓고 추후에 추가하도록 하겠습니다.

backend/src/test/java/codezap/template/service/TemplateServiceTest.java

backend/src/test/resources/clear.sql

kyum-q

개발 할 때 열띤 토론을 하시던데, 그 과정이 아깝지 않을 정도로 코드 굿 😊 수고하셨습니다 👍🏻

코멘트로 제 의견을 좀 남겨놨어요 ! 확인해보시고 괜찮다고 생각하는 의견만 수용해주세요. (모두 다 수용하실 필요 없습니다 ~! )

kyum-q · 2024-07-20T04:55:58Z

backend/src/main/java/codezap/global/exception/GlobalExceptionHandler.java

+@RestControllerAdvice
+public class GlobalExceptionHandler {
+
+    @ExceptionHandler


사소하지만 @ExceptionHandler를 @ExceptionHandler(CodeZapException.class) 로 해당 메서드가 어떤 exception을 처리하는지 명시해주면 좋을 것 같아요 😊

메서드명과 매개변수만으로도 충분히 어떤 exception을 처리하는지 알 수 있다고 생각해서 불필요한 코드라고 판단하였습니다.

backend/src/main/java/codezap/global/exception/GlobalExceptionHandler.java

backend/src/main/java/codezap/template/controller/TemplateController.java

backend/src/main/java/codezap/template/domain/Snippet.java

kyum-q · 2024-07-20T05:18:39Z

backend/src/main/java/codezap/template/domain/Template.java

-    @ManyToOne
-    @JoinColumn(name = "member_id", nullable = false)
-    private Member member;
-
    @Column(nullable = false)


join된 컬럼에만 이름 설정을 해줬네요! 그냥 컬럼에는 이름 설정을 하지 않은 이유가 있을까요?
저 같은 경우에는 코드의 일관성, 읽기 편리함, 변수명 변경에 컬럼에 영향을 주지 않음 과 같은 이유로 컬럼이 변수 명과 동일하더라도, 이름 설정을 따로 해주는 편이라 궁금해서 물어봅니당 ~

원래 의도는 필드 이름과 다르게 저장되는 컬럼 이름만 명시해 주고자 하는 의도였어요.
하지만, 우리가 명시해 준 이름을 그대로 사용하는 경우 굳이 이름을 명시해 주는 것이 불필요하다고 느껴 제거해 주었습니다!

+) 변수 명이 변경이 된다면 컬럼에 영향을 주는 것이 자연스럽다고 생각해요. 영향을 주고 싶지 않을 경우 그 때 이름을 명시해 주는 것이 좋아 보이기도 하고요!

kyum-q · 2024-07-20T05:21:21Z

backend/src/main/java/codezap/template/dto/request/CreateSnippetRequest.java

        String content,
+        @NotNull(message = "스니펫 순서가 null 입니다.")


null 입니다 라는 문장은 너무 개발자 측면의 문장이라고 생각합니다. 입력되지 않았습니다. 와 같은 문장은 어떤가요?

이거는 프론트엔드와 이야기했었던 내용인데
어차피 프론트 측에서 별도의 메세지로 다시 한 번 래핑해서 내보낸다고 해서 프론트 쪽으로 보내는 의미 정도로만 작성하였습니다!!

kyum-q · 2024-07-20T05:39:19Z

backend/src/main/java/codezap/template/dto/response/FindAllTemplatesResponse.java


 public record FindAllTemplatesResponse(
        List<FindTemplateBySummaryResponse> templates
 ) {
-    public static FindAllTemplatesResponse from(List<RepresentativeSnippet> representativeSnippets) {
-        List<FindTemplateBySummaryResponse> templatesBySummaryResponse = representativeSnippets.stream()
+    public static FindAllTemplatesResponse from(List<ThumbnailSnippet> thumbnailSnippets) {


dto 네이밍은 FindAllTemplatesResponse인데 실제 안에 들어있는 변수는 SummaryResponse이고, SummaryRespnse는 ThumbnailSnippet의 dto 네요.
SummaryRespnse 명이 ThumbnailSnippet의 dto임을 잘 나타내는 이름으로 네이밍 된다면 좋을 것 같아요😊

그리고 이건 갑자기 든 생각인데, ThumbnailSnippet 보다 ThumbnailTemplate은 어떤가요?
연속 사진에서 섬네일 사진은 대표 사진인 것처럼, 템플릿에 섬네일 ! ㅎㅎ
이렇게 되면 네이밍이 자연스러울 것 같아서요
( FindAllTemplatesResponse, ThumbnailTemplateResponse, ThumbnailTemplate )

FindTemplateBySummaryResponse 클래스는 명확한 네이밍을 차지 못해서 FindAllTemplatesById의 내부 클래스로 변경하였습니다.

backend/src/main/java/codezap/template/dto/response/FindTemplateByIdResponse.java

backend/src/test/java/codezap/template/service/TemplateServiceTest.java

zeus6768

해커톤 이후 첫 PR이네요 ㅎㅎ 짱수 초롱 정말 고생 많았어요!! 🎉

첫 코드리뷰라 재미있기도, 조심스럽기도 했네요.

같이 얘기해봐요~

zeus6768 · 2024-07-20T07:46:24Z

backend/src/main/java/codezap/global/exception/GlobalExceptionHandler.java

+    @ExceptionHandler
+    public ResponseEntity<ProblemDetail> handleException(Exception exception) {
+        return ResponseEntity.internalServerError()
+                .body(ProblemDetail.forStatusAndDetail(HttpStatus.INTERNAL_SERVER_ERROR, exception.getMessage()));


이 메서드가 없어도 Exception이 발생하면 스프링에서 500에러를 주지 않나요? 본 메서드가 필요한 이유를 알고 싶습니다~

해당 메서드는 예기치 못한 오류가 발생했을 때를 구분하기 위해 작성하였습니다.
따라서 message 부분을 조금 더 명시적으로 나타낼 수 있도록 수정하였습니다.

backend/src/main/java/codezap/global/serializing/DateTimeFormatConfiguration.java

backend/src/main/java/codezap/template/domain/Snippet.java

zeus6768 · 2024-07-20T08:08:26Z

backend/src/main/java/codezap/template/dto/request/CreateTemplateRequest.java

+        @NotNull(message = "템플릿 이름이 null 입니다.")
+        @Size(max = 255, message = "템플릿 이름은 최대 255자까지 입력 가능합니다.")
        String title,
-        int representative_snippet_ordinal,
+        @NotNull(message = "스니펫 리스트가 null 입니다.")


https://github.com/woowacourse-teams/2024-code-zap/pull/94/files?diff=split&w=0#r1685259424

켬미가 리뷰해준 내용을 여기서도 생각해보면 좋을 것 같습니당

해당 커멘트에 답변 달아두었습니다!!

zeus6768 · 2024-07-20T08:10:41Z

backend/src/main/java/codezap/template/dto/request/CreateSnippetRequest.java

+        @NotNull(message = "파일 내용이 null 입니다.")
        String content,


filename 처럼 content의 max size도 여기서 검증해주는 건 어떻게 생각하시나요?

MySQL의 TEXT 타입은 65,535byte까지 저장할 수 있습니다! 출처 | MySQL Data Type Storage Requirements

해당 내용은 이번 PR 이 아닌 다른 이슈 #102 로 등록하여 해결하겠습니다!

backend/src/main/java/codezap/template/service/TemplateService.java

zeus6768 · 2024-07-20T08:52:50Z

backend/src/test/java/codezap/template/TemplateIntegrationTest.java

+    @Test
+    @DisplayName("템플릿 생성 성공")
+    void createTemplateSuccess() {
+        CreateTemplateRequest templateRequest = new CreateTemplateRequest("a".repeat(255),
+                List.of(new CreateSnippetRequest("a".repeat(255), "content", 1)));
+        RestAssured.given().log().all()
+                .contentType(ContentType.JSON)
+                .body(templateRequest)
+                .when().post("/templates")
+                .then().log().all()
+                .statusCode(201);
+    }


이 테스트가 템플릿 생성 성공 케이스를 검증하는 게 맞을까요?

만약 컨트롤러에서 201을 반환하고 실제 DB에 반영하지는 않는 경우, 이 테스트에서 검증할 수 있을까요?

서비스 로직을 함께 검증하고 싶다면, 모킹한 DB에서 데이터를 꺼내 확인하는 assert문이 필요할 것 같아요.

서비스 로직과 DB 로직을 함께 검증하고 싶다면, 로컬 DB에서 데이터를 꺼내는 로직이 필요할 것 같구요.

아니면 애초에 컨트롤러가 올바른 응답을 주는지만 검증하고 싶었던 걸까요?

테스트 범위를 알 수 있게 개선하면 좋을 것 같습니다!

서비스에 대한 테스트를 이미 진행하였으므로, 서비스는 정상 동작한다고 가정하였습니다.
이에 따라 해당 테스트 코드에서는 응답만을 확인하고 있습니다.

backend/src/test/java/codezap/template/TemplateIntegrationTest.java

zeus6768 · 2024-07-20T09:02:31Z

backend/src/test/java/codezap/template/service/TemplateServiceTest.java

+import codezap.template.dto.response.FindTemplateByIdResponse;
+import io.restassured.RestAssured;
+
+@SpringBootTest(webEnvironment = WebEnvironment.RANDOM_PORT)


@SpringBootTest는 스프링 컨테이너에 모든 빈을 로드해서 속도가 가장 느린 방법으로 알고 있어요.

서비스의 단위테스트를 의도했다면 @SpringBootTest를 사용하지 않거나, classes 속성으로 사용할 빈만 등록하는 건 어떨까요?

조금 더 찾아보고 추후에 적용하도록 하겠습니다

해당 이슈 태그해주세용~!

Co-authored-by: hoeseong123 <[email protected]>

kyum-q

수고하셨습니다~ 👍

jminkkk

수고하셨습니다🔥
리뷰가 잘 반영된 것 같아 approve 할게요~!

Co-authored-by: hoeseong123 <[email protected]>

HoeSeong123

코드잽 파이팅~~!! ⚡

jminkkk

마지막까지 파이팅.........!

jminkkk · 2024-07-23T05:58:01Z

backend/src/main/java/codezap/global/exception/GlobalExceptionHandler.java

+        BindingResult bindingResult = exception.getBindingResult();
+        return ResponseEntity.status(HttpStatus.BAD_REQUEST)
+                .body(ProblemDetail.forStatusAndDetail(HttpStatus.BAD_REQUEST,
+                        bindingResult.getFieldError().getDefaultMessage()));


getFieldError() 는 하나의 필드 에러만 가져오네요.
하나의 요청 바디에서 여러 개의 검증 오류가 난다면, 클라이언트는 여러번에 걸쳐서 에러를 해결해야 할 것 같아요.

유효하지 않은 필드가 여러 개라도 한번에 보여주는 게 어떨까요?
( getFieldErrors()로 수정 후 유효하지 않은 필드를 joining 해서 응답을 줄 수 있을 것 같아요. )

Co-authored-by: hoeseong123 <[email protected]>

HoeSeong123

코드잽 파이팅 ⚡

zeus6768

초롱 짱수 너무 고생 많았어요 !!!

jminkkk and others added 14 commits July 17, 2024 17:43

chore: 스프링독 설정 파일 작성

b7ca3cc

refactor(codezap): ERD 변경에 따른 불필요한 도메인 삭제

51bb109

Co-authored-by: hoeseong123 <[email protected]>

refactor(domain): 의미 있는 상수 추출

33ebac5

Co-authored-by: hoeseong123 <[email protected]>

refactor(codezap): global 패키지 하위 항목을 용도에 따라 패키징

4bb7794

Co-authored-by: hoeseong123 <[email protected]>

refactor(codezap): 패키지 이름 변경(thumbnail_snippet에서 언더바 제거)

35a741f

Co-authored-by: hoeseong123 <[email protected]>

feat(exception): 커스텀 exception 추가

86805f1

Co-authored-by: hoeseong123 <[email protected]>

refactor(domain): Table 어노테이션 삭제

ce9c803

Co-authored-by: hoeseong123 <[email protected]>

refactor(domain): ThumbnailSnippet의 template 속성에 not null 제약조건 추가

7e43676

Co-authored-by: hoeseong123 <[email protected]>

chore: restAssured 의존성 추가

7af7f99

Co-authored-by: hoeseong123 <[email protected]>

test(service): TemplateSerivce 테스트 코드 추가

a98ed61

Co-authored-by: hoeseong123 <[email protected]>

chore: validation 의존성 추가

ea6a80d

Co-authored-by: hoeseong123 <[email protected]>

refactor(template): Snippet 도메인을 Template 하위로 이동

c8d8db8

Co-authored-by: hoeseong123 <[email protected]>

feat(codezap): 템플릿 DTO 유효성 검증 및 테스트 추가

3ed0c06

Co-authored-by: hoeseong123 <[email protected]>

refactor(template): ThumbnailSnippet 도메인을 Template 하위로 이동

83d85ff

Co-authored-by: hoeseong123 <[email protected]>

zangsu added refactor 요구사항이 바뀌지 않은 변경사항 BE 백엔드 labels Jul 19, 2024

zangsu added this to the 2차 스프린트 🐣 milestone Jul 19, 2024

zangsu requested review from zeus6768, jminkkk, kyum-q and HoeSeong123 July 19, 2024 08:10

zangsu assigned zangsu and HoeSeong123 Jul 19, 2024

jminkkk requested changes Jul 20, 2024

View reviewed changes

Jaymyong66 reviewed Jul 20, 2024

View reviewed changes

backend/src/test/resources/clear.sql Show resolved Hide resolved

kyum-q requested changes Jul 20, 2024

View reviewed changes

zeus6768 requested changes Jul 20, 2024

View reviewed changes

zangsu and others added 3 commits July 22, 2024 13:12

chore: 의존성 구분을 위해 개행 추가

00c5586

Co-authored-by: hoeseong123 <[email protected]>

refactor(serialization): 패키지 이름 변경

55f793e

Co-authored-by: hoeseong123 <[email protected]>

refactor(template): Location 헤더 수정 및 테스트 추가

ed328e7

Co-authored-by: hoeseong123 <[email protected]>

zangsu and others added 9 commits July 22, 2024 17:25

refactor(domain): 불필요한 JoinColumn 제거

7466794

Co-authored-by: hoeseong123 <[email protected]>

refactor(response): DTO 이름을 직관적으로 변경 및 내부 클래스로 이동

ce60635

Co-authored-by: hoeseong123 <[email protected]>

refactor(response): 파라미터를 2개 이상 받는 정팩메 이름을 of 로 변경

0a4f42f

Co-authored-by: hoeseong123 <[email protected]>

refactor(exception): 예외 메시지 변경

43e2bb3

Co-authored-by: hoeseong123 <[email protected]>

refactor(integration): 메서드 사이에 개행 추가

242708f

Co-authored-by: hoeseong123 <[email protected]>

refactor(template): id를 받지 않는 도메인 생성자에

aefb907

Co-authored-by: hoeseong123 <[email protected]>

test(service): repository 의존성 추가 및 테스트 코드 변경

e939f42

Co-authored-by: hoeseong123 <[email protected]>

refactor(integration): 최대 길이 상수화

56a8a77

Co-authored-by: hoeseong123 <[email protected]>

refactor(serialization): timezone 설정 제거

e5965d5

Co-authored-by: hoeseong123 <[email protected]>

HoeSeong123 previously approved these changes Jul 23, 2024

View reviewed changes

kyum-q previously approved these changes Jul 23, 2024

View reviewed changes

jminkkk previously approved these changes Jul 23, 2024

View reviewed changes

refactor(controller): IntegrationTest를 ControllerTest로 이름 변경

89cafb0

Co-authored-by: hoeseong123 <[email protected]>

zangsu dismissed stale reviews from jminkkk, kyum-q, and HoeSeong123 via 89cafb0 July 23, 2024 05:44

HoeSeong123 previously approved these changes Jul 23, 2024

View reviewed changes

jminkkk approved these changes Jul 23, 2024

View reviewed changes

kyum-q previously approved these changes Jul 23, 2024

View reviewed changes

jminkkk requested changes Jul 23, 2024

View reviewed changes

zeus6768 previously approved these changes Jul 23, 2024

View reviewed changes

refactor(exception): MethodArgumentNotValid 예외처리 메서드 NPE 경고 해결

8b3de77

Co-authored-by: hoeseong123 <[email protected]>

zangsu dismissed stale reviews from zeus6768, kyum-q, and HoeSeong123 via 8b3de77 July 23, 2024 06:08

jminkkk approved these changes Jul 23, 2024

View reviewed changes

kyum-q approved these changes Jul 23, 2024

View reviewed changes

HoeSeong123 approved these changes Jul 23, 2024

View reviewed changes

zeus6768 approved these changes Jul 23, 2024

View reviewed changes

zeus6768 merged commit a115ea4 into woowacourse-teams:dev/be Jul 23, 2024
1 check passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

해커톤 작업 내역을 리팩토링 #94

해커톤 작업 내역을 리팩토링 #94

zangsu commented Jul 19, 2024

jminkkk left a comment

jminkkk Jul 19, 2024

HoeSeong123 Jul 22, 2024

kyum-q left a comment •

edited

Loading

kyum-q Jul 20, 2024

HoeSeong123 Jul 22, 2024

kyum-q Jul 20, 2024

HoeSeong123 Jul 22, 2024

kyum-q Jul 20, 2024

HoeSeong123 Jul 21, 2024

kyum-q Jul 20, 2024

HoeSeong123 Jul 22, 2024

zeus6768 left a comment

zeus6768 Jul 20, 2024

HoeSeong123 Jul 22, 2024 •

edited

Loading

zeus6768 Jul 20, 2024

HoeSeong123 Jul 22, 2024

zeus6768 Jul 20, 2024

zangsu Jul 22, 2024

zeus6768 Jul 20, 2024

HoeSeong123 Jul 22, 2024

zeus6768 Jul 20, 2024

zangsu Jul 22, 2024

zeus6768 Jul 23, 2024

kyum-q left a comment

jminkkk left a comment

HoeSeong123 left a comment

jminkkk left a comment

jminkkk Jul 23, 2024

HoeSeong123 left a comment

zeus6768 left a comment

		String content,
		@NotNull(message = "스니펫 순서가 null 입니다.")

		@NotNull(message = "파일 내용이 null 입니다.")
		String content,

해커톤 작업 내역을 리팩토링 #94

해커톤 작업 내역을 리팩토링 #94

Conversation

zangsu commented Jul 19, 2024

⚡️ 관련 이슈

📍주요 변경 사항

🎸기타

jminkkk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kyum-q left a comment • edited Loading

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

zeus6768 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HoeSeong123 Jul 22, 2024 • edited Loading

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

kyum-q left a comment

Choose a reason for hiding this comment

jminkkk left a comment

Choose a reason for hiding this comment

HoeSeong123 left a comment

Choose a reason for hiding this comment

jminkkk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HoeSeong123 left a comment

Choose a reason for hiding this comment

zeus6768 left a comment

Choose a reason for hiding this comment

kyum-q left a comment •

edited

Loading

HoeSeong123 Jul 22, 2024 •

edited

Loading