-
Notifications
You must be signed in to change notification settings - Fork 28
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
[김진희] Good-Night-3rd-Hackathon-Backend 제출 #19
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
고생 많으셨습니다. 코드 읽어보면서 여러 가지 코멘트를 남겨봤는데요. 혹시 이해가 안되는 것이 있거나 질문이 있으시면 편하게 코멘트 남겨주시거나 DM 보내주세요.
추가로 커멘트 남길 것은:
- README를 작성하셔서 프로젝트 소개, 필요한 의존성, 셋업 방법 등을 적어주시면 좋을 것 같습니다.
- 코드를 예쁘게 정렬, 정리해주는 포매팅 툴이 따로 명시되어 있지 않은데, 이러면 여러 사람의 에디터 설정에 따라 정렬이 다르게 될 수 있습니다. google-java-format 같은 포매팅이 유명한데 이런 걸 에디터(vscode, IntelliJ)에서 셋업하는 방법을 알아보고, README에도 적어주시거나, 코드를 내려받으면 자동으로 셋업되도록 설정해주시면 좋을 것 같아요.
- 나중에 API 문서화를 위해 Swagger 같은 것도 찾아보시면 좋을 것 같아요.
그리고 문의 주신 내용에는요.
- 코드는 전반적으로 깔끔하게 잘 적어주신 것 같아요. 변수 네이밍이나 불필요한 필드 관련해서 코멘트 남겼는데 그거 반영해주시면 더 좋을 것 같아요.
- 프로젝트 규모가 작기도 하고, 지금 사용하신 구조도 많이 사용하는 방식이라 패키지 구조에서 흠 잡을 건 없었어요. 하지만 규모가 커졌을 때 어떤 식으로 관리하는지가 중요한데요. 여러 고민이 생길 거에요.
- 유틸성 코드(e.g.,
HeaderUtils
)는 어디서 관리할 것인가? - 인증(e.g., 카카오 로그인), 날씨, 결제 등과 같은 외부 의존성은 어디서 어떻게 관리할 것인가?
- 패키지 중첩은 몇 뎁스까지가 관리하기 적절한가?
- 어디까지 객체지향을 해야하는가?
- 유틸성 코드(e.g.,
이런 건 직접 해보셔야 어떤 상황에 어떤 걸 써야할지 더 와닿으실 것 같습니다. 실제로 구현하면서 구조를 바꾸기도 하구요.
dependencies { | ||
implementation 'org.springframework.boot:spring-boot-starter-data-jpa' | ||
implementation 'org.springframework.boot:spring-boot-starter-web' | ||
implementation 'org.antlr:antlr4-runtime:4.13.0' | ||
compileOnly 'org.projectlombok:lombok' | ||
runtimeOnly 'com.h2database:h2' | ||
runtimeOnly 'com.mysql:mysql-connector-j' | ||
annotationProcessor 'org.projectlombok:lombok' | ||
testImplementation 'org.springframework.boot:spring-boot-starter-test' | ||
testRuntimeOnly 'org.junit.platform:junit-platform-launcher' | ||
implementation 'jakarta.persistence:jakarta.persistence-api:3.1.0' | ||
implementation 'org.hibernate.orm:hibernate-core:6.2.6.Final' | ||
implementation 'org.springframework.boot:spring-boot-starter-data-jpa' | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
build.gradle
의 dependencies
에는 프로젝트가 의존하는 라이브러리나 툴이 들어가는데요.
스프링이 아니더라도 모든 프로젝트에서는 라이브러리의 경우 버전의 명시가 중요합니다. 버전을 명시하지 않으면 보통 latest (최신 버전)으로 돌아가는데, 이러면 프로젝트를 셋업/빌드할 때마다(= 배포할 때마다) 버전이 달라질 수 있어서, 진희님이 처음 프로젝트를 셋업했을 땐 실행이 잘 되었더라도 가령 내일도 작동한다는 보장을 할 수가 없습니다.
현존하는 대부분 라이브러리는 Semantic versioning이라는 규칙을 암묵적으로 따라 가고 있습니다.
주.부.수
// 예: 1.0.0, 2.2.1, ...
- 기존 버전과 호환되지 않는 변화가 있을 때 주(Major)를 바꾼다.
- 기존 버전과 호환되는 새로운 기능을 추가할 때 부(Minor)를 바꾼다.
- 기존 버전과 호환되는 버그 수정의 경우 수(Patch)를 바꾼다.
특히 주버전의 변화에 주의해야 합니다. 예를 들면 Spring Boot 2와 3은 JDK 최소 버전이 달라서 2에서는 낮은 버전을 사용할 수 없어요. (물론 라이브러리가 꼭 이를 따른다고 볼 수 없고, 버그가 있을 수 있기 때문에 부, 수버전도 주의해야 합니다.)
따라서 라이브러리 이름 뒤에 :버전
을 명시해주는 것이 좋겠습니다.
라이브러리의 버전별 사용 현황은 https://mvnrepository.com/ 에서 라이브러리 검색 후 확인하실 수 있습니다.
try { | ||
Comments createdComment = commentsService.createComment(wishId, commentsDto.getContent()); | ||
return ResponseEntity.status(HttpStatus.CREATED).body(createdComment); | ||
} catch (IllegalArgumentException e) { | ||
String errorMessage = e.getMessage(); | ||
if (errorMessage.equals("해당 소원이 존재하지 않습니다.")) { | ||
return ResponseEntity.status(HttpStatus.NOT_FOUND).body(errorMessage); | ||
} else if (errorMessage.equals("삭제된 소원입니다.") || errorMessage.equals("승인되지 않은 소원입니다.")) { | ||
return ResponseEntity.status(HttpStatus.BAD_REQUEST).body(errorMessage); | ||
} else { | ||
return ResponseEntity.status(HttpStatus.INTERNAL_SERVER_ERROR).body(""); | ||
} | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
try-catch도 빠르게 구현할 때 나쁜 방법은 아니지만 단점이 좀 보이는데요.
- 컨트롤러에서 대부분 상황에서 실행되는 정상 케이스(
try { ... }
)에 집중하기엔 비정상 케이스(catch () { ... }
)의 코드가 더 길어서 하고자 하는 목적이 쉽게 눈에 들어오지 않습니다. - 매번 try-catch로 에러를 처리하는 것이 번거롭기도 하구요.
- 동일한 유형의 에러가 사용될 경우가 있을텐데 그때마다 코드가 중복됩니다.
이걸 정리하는 데에는 여러 가지 방법이 있습니다. 방법들을 하나씩 보시고 상황에 맞는걸 쓰시면 좋을 것 같아요. 앱 전체에서 Global하게 처리하는 방식도 있습니다.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Exception handling 처리와 함께 올바른 상태 코드를 내려주신 것은 좋습니다.
하지만 현재 retuirn ResponseEntity.status(에러_HTTP_상태_코드).body(문자열)
과 같이 body
에 문자열(String)을 바로 넘기면, 응답으로 Json 형태가 아닌, 헤더에 Content-Type: text/plain
가 들어간 일반 문자열이 나옵니다.
그러면 이걸 받는 클라이언트(모바일, 웹, ...)에서는 JSON이 아닌 text로 핸들링해야 하는데, 매끄러운 방식은 아닙니다.
통일된 형태의 에러 메시지를 담기 위한 DTO를 하나 만들어 주시면 더 좋을 것 같아요. 이러면 클라이언트에서 JSON 데이터를 파싱한 후 상황에 따른 올바른 처리를 할 수 있겠죠. 아래는 간단한 예시입니다.
{
"errorCode": "ERR_01",
"message": "해당 소원이 존재하지 않습니다."
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
서비스 단에서 에러를 일괄적으로 IllegalArgumentException
로 던져서, 컨트롤러에서 try-catch로 exception을 잡은 뒤, exception에 들어 있는 문자열을 읽고 if 문을 이용해 처리를 하고 계신데요.
간단한 상황에선 괜찮지만, 프로그램을 유지보수하다보면 골치 아픈 상황이 많이 발생합니다.
- 에러 메시지가 수정되면, 에러를 던지는 쪽과 읽고 받는 쪽 둘 다 수정을 해야 한다.
- 또 다른 에러 타입이 추가되었을 때, else if로 추가하지 않으면 무조건 500 Internal Server Error으로 처리된다. (에러 메시지가 없는 것은 덤)
상황에 맞는 커스텀 에러 클래스를 만들어서 대신 던져보면 어떨까요? 필요에 따라 에러에 맞는 상태코드와 JSON 형태까지 전달할 수 있습니다. 위에서 코멘트 드린 것들이 어느 정도 커버가 되는 것이죠.
구글에 spring custom error 같은 식으로 검색해보시면 관련 자료가 많이 나올거에요.
@RestController | ||
@RequestMapping("/wishes/{wishId}/comments") | ||
public class CommentsController { | ||
private final CommentsService commentsService; | ||
|
||
public CommentsController(CommentsService commentsService) { | ||
this.commentsService = commentsService; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Comment 컨트롤러에서 Comment 서비스를 의존성 주입(DI)하기 위해 생성자 방식을 사용하고 계신데요.
Lombok을 설치하신 김에 여기에 써볼까요? 생성자를 생략할 수 있습니다.
@RestController | |
@RequestMapping("/wishes/{wishId}/comments") | |
public class CommentsController { | |
private final CommentsService commentsService; | |
public CommentsController(CommentsService commentsService) { | |
this.commentsService = commentsService; | |
} | |
import lombok.RequiredArgsConstructor; | |
@RestController | |
@RequestMapping("/wishes/{wishId}/comments") | |
@RequiredArgsConstructor | |
public class CommentsController { | |
private final CommentsService commentsService; |
@RequiredArgsConstructor
는 final
, @NotNull
이 붙은 필드(우리의 경우 commentsService
)들을 모은 생성자를 자동 생성해주는 어노테이션입니다.
생성된 결과물 코드는 똑같겠지만 Lombok을 이용하면 생성자를 매번 생성하는 번거로움이 줄어들겠죠.
자바+스프링에서의 의존성 주입에는 @RequiredArgsConstructor
를 애용하시면 편하실 거에요.
public class CommentsDto { | ||
private Long wishId; | ||
private String content; | ||
private LocalDateTime createdAt; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
createdAt
과 같이 자동으로 생성할 수 있는 걸 API를 통해 받을 필요는 없을 것 같아요. 여기선 지우고 서비스에서 LocalDateTime.now()
을 쓰고 계시던데 그걸 쓰는게 어떨까요?
@Getter | ||
@Setter | ||
public class CommentsDto { | ||
private Long wishId; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wishId
는 컨트롤러에서 @Path
로 받고 있을텐데 여기선 필요 없을 것 같아요.
@Enumerated(EnumType.STRING) // 열거형 -> DB에 문자열로 저장 | ||
@Column(nullable = false) | ||
private Category category; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
enum 사용 좋습니다. 지금은 이걸로 충분한데 나중에 enum을 테이블에 저장하고 싶으실 때 enum converter라는 것도 찾아보시면 좋을 것 같아요.
|
||
@Getter | ||
@Setter | ||
public class CommentsDto { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
요청을 위한 DTO를 정의해주신 것 같은데요.
@NotNull
, @NotEmpty
, @NotBlank
, @Size
, ... 같은 validation을 위한 다양한 어노테이션이 있어서, DTO에도 좀 더 안전하게 가져가고 싶으시면 이런 것도 참고해보세요. 검증에 실패했을 때의 에러 메시지를 넣을 수도 있습니다.
|
||
@ManyToOne(fetch = FetchType.LAZY) | ||
@JoinColumn(name = "wishId", nullable = false) | ||
private Wishes wish; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ORM 쓰시는 것 좋지만 그냥 private Long wishId
를 넣기도 합니다. 이런 방법도 유효하다는 것 참고바라요.
implementation 'org.springframework.boot:spring-boot-starter-web' | ||
implementation 'org.antlr:antlr4-runtime:4.13.0' | ||
compileOnly 'org.projectlombok:lombok' | ||
runtimeOnly 'com.h2database:h2' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
H2가 프로토타이핑에는 괜찮지만 실제로는 쓰지 않습니다.
H2가 in-memory와 file 방식 두 개를 지원하는 것으로 아는데, 굳이 실 서버에 이런 방식으로 쓸 거라면 SQLite를 더 추천드릴게요.
아니면 그냥 PostgreSQL 같은 검증된 DB 서버를 쓰는 게 좋을 것 같습니다.
|
||
java { | ||
toolchain { | ||
languageVersion = JavaLanguageVersion.of(17) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
JDK 17, 스프링 부트 3, Gradle 8.8을 쓰시는 것으로 보이는데요.
이 세개는 버전에 따라 조금씩 사용할 수 있는 것들이 달라서요.
실무에서 최신이 아닌 여러 버전을 쓸 수 있으니 차이점을 어느 정도 숙지하시고 서로 호환되는 여러 조합을 경험해보시면 좋을 것 같습니다. (e.g. JDK 8, 11, 17, 21, Gradle 6, 7, 8, ...)
SpringBoot로 구현하였습니다😁