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

[후] 스프링 카페 3단계 - DB에 저장하기 #129

Open
wants to merge 16 commits into
base: who-hoo
Choose a base branch
from

Conversation

who-hoo
Copy link

@who-hoo who-hoo commented Mar 18, 2022

안녕하세요! 스프링 카페 3단계 PR 요청 드립니다. 항상 감사합니다 :)

변경 사항

@who-hoo who-hoo added the review-BE Improvements or additions to documentation label Mar 18, 2022
@wheejuni wheejuni self-requested a review March 20, 2022 12:13
@wheejuni wheejuni self-assigned this Mar 20, 2022
Copy link

@wheejuni wheejuni left a comment

Choose a reason for hiding this comment

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

대단히 수고 많으셨습니다. 💯
몇가지 생각해보시면 좋을 점 코멘트로 남겨드립니다~

@@ -5,31 +5,46 @@

public class Article {

private int id;
private String userId;
private final Integer id;

Choose a reason for hiding this comment

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

idInteger 로 저장하는 것.. 괜찮을까요?
괜찮다고 쉽게 생각할 수는 있지만 권장하지는 않습니다. 왜 권장하지 않을까요?

Comment on lines +33 to +34
public Article(Integer id, String userId, String title, String content,
AtomicInteger viewCount, LocalDateTime createdAt) {

Choose a reason for hiding this comment

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

viewCountAtomicInteger 인 이유도 궁금해요.

this.email = email;
this.userId = userId;
this.password = password;
this.createdAt = LocalDateTime.now();
}

public User(int id, User user) {

Choose a reason for hiding this comment

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

Integerint 사이에 변환이 일어날 때 생각해볼 만한 리스크는 없을까요?

import org.springframework.stereotype.Repository;

@Repository
public class H2ArticleRepository implements ArticleRepository {

Choose a reason for hiding this comment

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

클래스명에 특정 기술/라이브러리/프레임워크가 들어가는 것에 강하게 반대합니다.
이렇게 특정 기술에 종속되지 않으려고 클래스를 나누고, 인터페이스를 분리하는 것이라고 생각합니다.
그리고 H2MySQL 이 레포지토리가 달라야 하는 이유도 잘 모르겠습니다.
그 둘은 완벽하게 호환되어서 서로의 존재를 몰라도 로컬 테스트/디버깅이 용이하게끔 사용하는 목적이 있습니다.
왜 달라야 하죠?

Comment on lines +31 to +35
public int save(Article article) {
if (article.isNewArticle()) {
return insert(article);
}
return update(article);

Choose a reason for hiding this comment

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

있거나 없거나 하나의 쿼리로 처리하는 방식은 없으려나요?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
review-BE Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants