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

[14주차] 심규창 학습 PR 제출합니다. #1

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

gyuchangShim
Copy link

@gyuchangShim gyuchangShim changed the title COW JPA Practice (심규창) [14주차] 심규창 학습 PR 제출합니다. Jan 2, 2024
Comment on lines 8 to +9
username: root
password: mysql
password: tlarbckd@17
Copy link

@k-kbk k-kbk Jan 3, 2024

Choose a reason for hiding this comment

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

이런 정보는 보안에 매우 중요한 부분이니 환경변수화해서 외부에 감추면 좋을 것 같아요!

@k-kbk
Copy link

k-kbk commented Jan 3, 2024

전반적으로 정말 잘 작성하신 것 같아요! 👍
고생하셨어요!


import java.util.List;

@RestController
Copy link
Member

Choose a reason for hiding this comment

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

@RestContoller@Controller 알아보기!

import java.util.List;

@RestController
@RequestMapping("api/boards")
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
@RequestMapping("api/boards")
@RequestMapping("/api/boards")

src/main/java/com/example/jpa/board/domain/Board.java Outdated Show resolved Hide resolved
src/main/java/com/example/jpa/board/domain/Board.java Outdated Show resolved Hide resolved
Comment on lines 9 to 11
private String name;
private int age;
private String phoneNumber;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
private String name;
private int age;
private String phoneNumber;
private final String name;
private final int age;
private final String phoneNumber;

Comment on lines +24 to +26
@ManyToOne(fetch = FetchType.LAZY)
@JoinColumn(name = "board_id")
private Board board;
Copy link
Member

Choose a reason for hiding this comment

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

양방향 매핑 지양

Copy link
Member

Choose a reason for hiding this comment

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

왜 양방향 매핑을 지양해야 할까요? 특히 Jpa엔티티와 도메인이 같은 객체일 경우에는 일어날 수 있는 문제점이 정말 많습니다.
객체간 순환참조가 될 뿐만 아니라, 조금만 도메인이 많아져도 양방향 지옥에 빠져 어려움을 겪을 확률이 너무너무 큽니다... 관리도 안될 뿐더러 의도치 않는 쿼리가 나갈 수도 있고요.
정말 두 객체가 서로 알고 있어야하는지 고민해볼 필요가 있을 것 같아요.

Comment on lines +51 to 55
public Member checkExist(Long id) {
Member member = memberRepository.findById(id)
.orElseThrow(() -> new NoSuchElementException("멤버가 존재하지 않습니다."));

return MemberResponse.from(member);
return member;
}
Copy link
Member

Choose a reason for hiding this comment

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

public이 적절한지 고민해보기.
check 메서드의 동작 방식 통일하기(조건 실패시 에러 반환, boolean 반환 등)

Copy link
Author

Choose a reason for hiding this comment

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

모든 checkExist 메서드의 동작을 객체 반환으로 통일했습니다. 해당 메서드는 MemberService 뿐만 아니라 Board, Reply의 동작에도 사용해서 public으로 만들었습니다!!

Copy link
Member

@stophwan stophwan left a comment

Choose a reason for hiding this comment

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

👍 고생하셨습니다!! 코멘트들을 잘 확인해서 다음 미션때 반영해봐요 ㅎㅎ

Comment on lines 45 to 58
public static Board of(BoardCreateRequest boardCreateRequest, Member member) {
return Board.builder()
.member(member)
.title(boardCreateRequest.getTitle())
.content(boardCreateRequest.getContent())
.date(LocalDate.now())
.build();
}

public void update(BoardUpdateRequest boardUpdateRequest) {
this.title = boardUpdateRequest.getTitle();
this.content = boardUpdateRequest.getContent();
this.date = LocalDate.now();
}
Copy link
Member

Choose a reason for hiding this comment

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

도에인에 Request 객체가 있는 형태는 좋지 않은 방식입니다!
가장 큰 이유는 둘이 사이클 차이가 너무 큽니다. 변경 가능성이 많이 없는 Board 객체에 비해서 웹에 가장 앞에 있는 Request 객체는 변경 가능성이 높거든요. 요청이 변경될 때마다 도메인 객체를 변경해야하니까요.

Comment on lines 8 to 15
public class BoardCreateRequest {

@Column(name = "user_id")
private Long id;

private String title;
private String content;
}
Copy link
Member

Choose a reason for hiding this comment

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

Reqeust에서 검증 조건이 너무 없습니다. 당장 id나 title content가 null 값이 들어와도 그대로 다 받아버려요.
Column은 JPA에서 사용하는 어노테이션으로 알고 있는데 불필요해 보입니다.

Copy link
Author

Choose a reason for hiding this comment

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

Spring validation dependency 사용해서 검증 조건 추가했습니다!

src/main/java/com/example/jpa/board/domain/Board.java Outdated Show resolved Hide resolved
Comment on lines +50 to +54
public Board checkExist(Long id) {
Board targetBoard = boardRepository.findById(id)
.orElseThrow(() -> new NoSuchElementException("게시판이 존재하지 않습니다."));
return targetBoard;
}
Copy link
Member

@stophwan stophwan Jan 3, 2024

Choose a reason for hiding this comment

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

도진님 말씀 굉장히 동감합니다. 메서드가 직관적이지 않은것 가타용.

Comment on lines +24 to +26
@ManyToOne(fetch = FetchType.LAZY)
@JoinColumn(name = "board_id")
private Board board;
Copy link
Member

Choose a reason for hiding this comment

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

왜 양방향 매핑을 지양해야 할까요? 특히 Jpa엔티티와 도메인이 같은 객체일 경우에는 일어날 수 있는 문제점이 정말 많습니다.
객체간 순환참조가 될 뿐만 아니라, 조금만 도메인이 많아져도 양방향 지옥에 빠져 어려움을 겪을 확률이 너무너무 큽니다... 관리도 안될 뿐더러 의도치 않는 쿼리가 나갈 수도 있고요.
정말 두 객체가 서로 알고 있어야하는지 고민해볼 필요가 있을 것 같아요.

Board 엔티티의 of 메서드 대신 BoardCreateRequest에 toEntity 메서드로 대체 & MemberService의 checkDuplicate 메서드를 if -> Optional로 변경
Reply 엔티티 생성 과정에서 of 메서드 대신 DTO에 toEntity 메서드 추가 & Create, Update시 사용되는 DTO 객체의 속성들 검증 조건 추가
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants