-
Notifications
You must be signed in to change notification settings - Fork 13
[Spring Core] 배종연 과제 제출합니다. #9
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
base: main
Are you sure you want to change the base?
Conversation
songsunkook
left a comment
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.
이번 주 과제를 열심히 진행해주셨네요!
몇 가지 궁금한 부분 코멘트 남겨드렸습니다
고생하셨어요~
| compileOnly 'org.projectlombok:lombok' | ||
| annotationProcessor 'org.projectlombok:lombok' | ||
| testCompileOnly 'org.projectlombok:lombok' | ||
| testAnnotationProcessor 'org.projectlombok:lombok' |
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.
롬복을 도입한 이유와 도입할 때 어떤 점을 고려했는지 궁금합니다
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.
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으로 관리하니 깔끔하네요 👍
| public ErrorResponse(ErrorCode errorCode) { | ||
| this.code_id = errorCode.getCode_id(); | ||
| this.code = errorCode.getCode(); | ||
| this.message = errorCode.getMessage(); | ||
| } | ||
|
|
||
| public ErrorResponse(ErrorCode errorCode, String message) { | ||
| this.code_id = errorCode.getCode_id(); | ||
| this.code = errorCode.getCode(); | ||
| this.message = 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.
여러 생성자를 사용한다면 정적 팩토리 메서드로 대체해보는 건 어떻게 생각하세요?
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.
정적 팩토리 메소드라는 용어가 익숙하지 않아서 정적 팩토리 메소드에 대해 한번 공부하고 긍정적으로 고려해보겠습니다.
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.
어떤 예외를 잡아도 처리하는 과정이 동일하다면 극단적으로는 하나의 메서드로 묶어버릴 수도 있을 것 같아요.
메서드별로 다른 부분은 ErrorCode.Article_Not_Found와 같은 부분밖에 안보이는데, 이를 커스텀 익셉션의 필드로 선언해둔다면 모든 메서드의 로직을 통일할 수 있을 것 같습니다.
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.
커스텀 익셉션 필드에 대해 공부 후 변경해보겠습니다.
| Article findById(Long id); | ||
| Optional<Article> findById(Long id); |
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.
optional을 사용하신 이유가 있나요?
+) 코드 상 사용처가 있는지도 궁금합니다
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.
처음에는 Article로 IntelliJ에서 빨간줄과 함께 optional 사용을 권장한다고 떠서 사용했던것 같습니다.
src/main/resources/application.yml
Outdated
| url: jdbc:mysql://localhost:3306/bcsd # 본인의 환경에 맞게 수정한다. | ||
| username: root # 본인의 환경에 맞게 수정한다. | ||
| password: qwer1234 # 본인의 환경에 맞게 수정한다. | ||
| password: 1q2w3e4r! # 본인의 환경에 맞게 수정한다. |
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.
gitignore로 제외해주세요!
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.
제외하였습니다!
songsunkook
left a comment
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.
전반적으로 잘 짜주신 것 같아요. 과제 하느라 고생하셨습니다~ 👍
| @Override | ||
| public Article update(Article article) { | ||
| return em.merge(article); | ||
| } |
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.
entitymanager에 왜 update 메서드가 없는지 생각해보면 좋을 것 같아요
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.
찾아보니 더티체킹이라는 기능이 JPA에 있어서 update가 따로 존재하지 않는다고 합니다. 여기서 더티체킹이란 상태변경검사로 트랜젝션이 끝나는 시점에 변화가 있는 모든 엔티티 객체를 db에 자동으로 반영해준다고 합니다. 따라서 merge를 통해 값을 조회후 변경을 하면 더티체킹을 통해 db에 업데이트가 되기 때문에 update메소드가 따로 존재하지 않는것 같습니다.
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.
merge를 해야 더티체킹이 되는건가요?
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.
더티체킹은 상태변화를 감지하는 것이기 때문에 꼭 merge가 아니어도 자동으로 실행됩니다
| @@ -1,4 +1,4 @@ | |||
| package com.example.demo.repository; | |||
| /* package com.example.demo.repository; | |||
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.
클래스를 통째로 주석처리하기보다는 인터페이스로 분리되어있는 김에 다형성을 활용하면 더 좋았을 것 같아요
키워드: Primary, Qualifier
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.
질문이 있습니다. 제가 만약 @qualifier로 저 jdbc를 등록해놓으면 제가 qualifier로 지정한 이름을 부르지 않는 이상 저 코드들은 실행이 아예 되지 않는 건가요? 이번 과제에선 jpa를 사용하니 jpa를 primary로 지정하고 jdbc를 qualifier로 지정해서 평소에는 자동으로 jpa만 쓰다가 제가 jdbc가 필요할때 불러오는 방식으로 사용하는것이 맞나요?
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.
| @Override | ||
| public boolean existByAuthorId(Long authorId) { | ||
| return false; | ||
| } | ||
|
|
||
| @Override | ||
| public boolean existByBoardId(Long boardId) { | ||
| return false; | ||
| } |
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.
이건 아직 구현하지 못한 건가요?
songsunkook
left a comment
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.
잘 짜주셨네요
이번 주 과제도 고생하셨습니다~
| @Entity | ||
| @Table(name="article") | ||
| @Getter | ||
| @Setter |
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.
Setter는 사용하기 전에 깊게 고민해보시기를 추천드립니다. 한 번 찾아보시면 좋을 것 같아요
| @OneToMany(mappedBy = "board") | ||
| private List<Article> articles=new ArrayList<>(); |
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.
new ArrayList<>()를 초기값으로 할당해주지 않으면 어떤 문제가 생길까요?
| public void update(String name) { | ||
| this.name = name; | ||
| } |
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.
Setter가 있는데 이 메서드를 만들어둔 이유가 있나요?
| public void removeArticle(Article article) { | ||
| articles.remove(article); | ||
| } |
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.
게시글을 유저 엔티티를 통해 remove한다고 하면, 이 메서드를 호출하면 게시글도 삭제되기를 기대하는 것이 바람직해 보여요. 하지만 이대로는 이 메서드를 호출해도 DB에는 아무 영향이 가지 않을 것 같아요. 왜 그런지 찾아보시고 해결해보면 좋을 것 같습니다
과제 제출합니다.