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 제출합니다 #3

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions README.md
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
![제목 없음](https://github.com/COW-edu/jpa-practice/assets/68328998/ed24af9e-da05-4492-bc86-3cbc7821aac7)
Original file line number Diff line number Diff line change
@@ -0,0 +1,49 @@
package com.example.jpa.board.controller;

import com.example.jpa.board.domain.Board;
import com.example.jpa.board.dto.request.BoardCreateRequest;
import com.example.jpa.board.dto.request.BoardUpdateRequest;
import com.example.jpa.board.dto.response.BoardResponse;
import com.example.jpa.board.service.BoardService;
import lombok.RequiredArgsConstructor;
import org.springframework.http.ResponseEntity;
import org.springframework.web.bind.annotation.*;

import java.util.List;

@RestController
@RequestMapping("api/board")
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/board")
@RequestMapping("/api/board")

@RequiredArgsConstructor
public class BoardController {
private final BoardService boardService;

@PostMapping
public ResponseEntity<Void> create(@RequestBody BoardCreateRequest boardCreateRequest) {
boardService.create(boardCreateRequest);
return ResponseEntity.ok().build();
}

@GetMapping("/{id}")
public ResponseEntity<BoardResponse> findOne(@PathVariable("id") Long id) {
BoardResponse boardResponse = boardService.findOne(id);
return ResponseEntity.ok(boardResponse);
}

@GetMapping("/findAll")
public ResponseEntity<BoardResponse> findAll() {
List<BoardResponse> boardResponseList = boardService.findAll();
return ResponseEntity.ok((BoardResponse) boardResponseList);
}

@PutMapping("/{id}")
public ResponseEntity<BoardResponse> updateBoard(@RequestBody BoardUpdateRequest boardUpdateRequest) {
boardService.updateBoard(boardUpdateRequest);
return ResponseEntity.ok().build();
}

@DeleteMapping("/{id}")
public ResponseEntity<Void> deleteBoard(@PathVariable("id") Long id) {
boardService.deleteBoard(id);
return ResponseEntity.ok().build();
}
}
48 changes: 48 additions & 0 deletions src/main/java/com/example/jpa/board/domain/Board.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,48 @@
package com.example.jpa.board.domain;

import com.example.jpa.board.dto.request.BoardUpdateRequest;
import com.example.jpa.member.domain.Member;
import com.example.jpa.reply.domain.Reply;
import jakarta.persistence.*;
import lombok.*;

import java.util.ArrayList;
import java.util.List;

@Entity
@Getter
@Setter
@Builder
@NoArgsConstructor(access = AccessLevel.PROTECTED)
Comment on lines +12 to +16
Copy link
Member

Choose a reason for hiding this comment

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

지양해야하는 방식입니다. 저렇게 getter와 setter를 다 열어둔다면 필드를 Private으로 한 이유가 없습니다.

public class Board {

@Id
@GeneratedValue(strategy = GenerationType.IDENTITY)
private Long id;

private String title;
private String content;

private Long member_id;
Copy link
Member

Choose a reason for hiding this comment

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

값자기 스네이크 케이스를 쓰신 이유가 있나요?
밑에서 이미 연관관계를 맺고 있는데 추가로 이걸 작성하신 이유가 궁금합니다!


@ManyToOne(fetch = FetchType.LAZY)
private Member member;

@OneToMany
Copy link
Member

Choose a reason for hiding this comment

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

mappedBy 옵션이 없습니다!

private List<Reply> replyList;


public Board(Long id, String title, String content, Member member, List<Reply> replyList) {
this.id = id;
this.title = title;
this.content = content;
this.member = member;
this.replyList = replyList;
}

public void update(BoardUpdateRequest boardUpdateRequest) {
this.title = boardUpdateRequest.getTitle();
this.content = boardUpdateRequest.getContent();
this.member = boardUpdateRequest.getMember();
}
Comment on lines +43 to +47
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 객체는 변경 가능성이 높거든요. 요청이 변경될 때마다 도메인 객체를 변경해야하니까요.

}
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
package com.example.jpa.board.dto.request;

import com.example.jpa.board.domain.Board;
import com.example.jpa.member.domain.Member;
import lombok.Getter;

@Getter
public class BoardCreateRequest {
private String title;
private String content;
private Member member;
Comment on lines +9 to +11
Copy link
Member

Choose a reason for hiding this comment

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

title이 null이 들어오면 어떻게 될까요?
추가적인 제약조건이 필요합니다.
NotNull, NotEmpty, NotBlank의 차이에 대해서 학습 추천드려요.


public Board toEntity(Member member) {
return Board.builder()
.title(this.title)
.content(this.content)
.member(member)
Comment on lines +15 to +17
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
.title(this.title)
.content(this.content)
.member(member)
.title(title)
.content(content)
.member(member)

.build();
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
package com.example.jpa.board.dto.request;

import com.example.jpa.member.domain.Member;
import lombok.Getter;

@Getter
public class BoardUpdateRequest {
private Long id;
private String title;
private String content;
private Member member;
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
package com.example.jpa.board.dto.response;

import com.example.jpa.board.domain.Board;
import com.example.jpa.member.domain.Member;
import com.example.jpa.reply.domain.Reply;
import lombok.Getter;
import lombok.Setter;

import java.util.List;

@Getter
public class BoardResponse {
private Long id;
private String title;
private String content;
private Long memberId;


private String memberName;
private List<Reply> replyList;
Comment on lines +13 to +20
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 Long id;
private String title;
private String content;
private Long memberId;
private String memberName;
private List<Reply> replyList;
private Long id;
private String title;
private String content;
private Long memberId;
private String memberName;
private List<Reply> replyList;

Copy link
Member

Choose a reason for hiding this comment

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

도메인 전체를 바깥에 노출하는건 좋지 않습니다! Reply를 따로 감싸는걸 권장드려요.


public BoardResponse(Long id, String title, String content, Long memberId, String memberName, List<Reply> replyList) {
this.id = id;
this.title = title;
this.content = content;
this.memberId = memberId;
this.memberName = memberName;
this.replyList = replyList;
}

public static BoardResponse from(Board board) {
return new BoardResponse(board.getId(), board.getTitle(), board.getContent(), board.getMember().getId(), board.getMember().getName(), board.getReplyList());
Copy link
Member

Choose a reason for hiding this comment

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

board.getMember().getId() 처럼 할경에 실행하시면 이상한 점을 느끼실 수 있을꺼에요! 직접 확인해보시고 문제점이 무엇인지 파악해보는 것을 추천드려요! 키워드는 LazyLoading 입니다.

}
}
77 changes: 77 additions & 0 deletions src/main/java/com/example/jpa/board/service/BoardService.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,77 @@
package com.example.jpa.board.service;

import com.example.jpa.board.domain.Board;
import com.example.jpa.board.dto.request.BoardCreateRequest;
import com.example.jpa.board.dto.request.BoardUpdateRequest;
import com.example.jpa.board.dto.response.BoardResponse;
import com.example.jpa.member.domain.Member;
import com.example.jpa.member.dto.response.MemberResponse;
import com.example.jpa.reply.domain.Reply;
import com.example.jpa.repository.BoardRepository;
import com.example.jpa.repository.MemberRepository;
import com.example.jpa.repository.ReplyRepository;
import lombok.RequiredArgsConstructor;
import org.springframework.stereotype.Service;
import org.springframework.transaction.annotation.Transactional;

import java.util.List;
import java.util.NoSuchElementException;
import java.util.stream.Collectors;

import static java.util.Arrays.stream;

@Service
@RequiredArgsConstructor
public class BoardService {
private final BoardRepository boardRepository;
private final ReplyRepository replyRepository;
private final MemberRepository memberRepository;
Comment on lines +26 to +28
Copy link
Member

Choose a reason for hiding this comment

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

저는 개인적으로 BoardRepository를 호출할 수 있는 것은 BoardService라고 생각해요. 이 이야기는 뭐냐면 ReplyRepository에 BoardService가 직접 접근해 Reply를 가져와서 찾는 행위가 ReplyService에서 하는 것이 올바른 책임인가 생각이 듭니다.
ReplyService에서 ReplyRepository 사용하고 ReplyService를 호출하는 것이 더 나아보입니다.
만약 다른 MemberService와 같이 많은 서비스에서 Reply가 필요하다면 그때마다 해당 도메인에 Service레이어가 ReplyRepository를 의존해야하니까요.
물론 이런 경우에는 서비스 레이어간의 순환참조가 일어날 수도 있지 않느냐? 하는 의견이 있긴합니다만 저는 애당초 레이어간의 순환참조가 일어난다면 그건 애당초 잘못된 설계라고 생각합니다!

해당 내용에 대한 논의 글도 있으니 확인해보시면 좋을 것 같아요!
woowacourse/retrospective#15


@Transactional
public void create(BoardCreateRequest boardCreateRequest) {
Member member = memberRepository.findById(boardCreateRequest.getMember().getId())
.orElseThrow(() -> new NoSuchElementException("멤버가 존재하지 않습니다."));

boardRepository.save(boardCreateRequest.toEntity(member));
}

@Transactional(readOnly = true)
public BoardResponse findOne(Long id) {
Board board = checkBoard(id);

List<Reply> replyList = replyRepository.findByBoardId(id);
board.setReplyList(replyList);

return BoardResponse.from(board);
}

@Transactional(readOnly = true)
public Board checkBoard(Long id) {
Board board = boardRepository.findById(id)
.orElseThrow(() -> new NoSuchElementException("게시판 정보가 존재하지 않습니다."));
return board;
}
Comment on lines +48 to +53
Copy link
Member

Choose a reason for hiding this comment

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

check라는것은 일반적으로는 boolean을 반환합니다 검증용 void 메서드일 수 있고요. check 이름의 메서드지만 Board를 반환하는 것은 메서드만 보았을때 정확히 무슨 메서드인지 확인하기 어려워보입니다!


@Transactional(readOnly = true)
public List<BoardResponse> findAll() {
List<Board> boardListTemp = boardRepository.findAll();
Copy link
Member

Choose a reason for hiding this comment

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

변수명이 무엇을 뜻하는지 어렵습니다.

List<BoardResponse> boardResponseList = boardListTemp.stream().map(BoardResponse::from).toList();
return boardResponseList;
}

@Transactional
public void updateBoard(BoardUpdateRequest boardUpdateRequest) {
Board board = checkBoard(boardUpdateRequest.getId());
board.update(boardUpdateRequest);
}

@Transactional
public void deleteBoard(Long id) {
Board board = checkBoard(id);

List<Reply> replyList = replyRepository.findByBoardId(id);
board.setReplyList(replyList);
Copy link
Member

Choose a reason for hiding this comment

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

setter 대신 의미있는 메서드 명을 사용하면 어떨까요?


boardRepository.delete(board);
}
}
Original file line number Diff line number Diff line change
@@ -1,17 +1,15 @@
package com.example.jpa.member.controller;

import com.example.jpa.member.dto.request.MemberCreateRequest;
import com.example.jpa.member.dto.request.MemberUpdateRequest;
import com.example.jpa.member.dto.response.MemberResponse;
import com.example.jpa.member.service.MemberService;
import lombok.RequiredArgsConstructor;

import org.springframework.http.ResponseEntity;
import org.springframework.web.bind.annotation.GetMapping;
import org.springframework.web.bind.annotation.PathVariable;
import org.springframework.web.bind.annotation.PostMapping;
import org.springframework.web.bind.annotation.RequestBody;
import org.springframework.web.bind.annotation.RequestMapping;
import org.springframework.web.bind.annotation.RestController;
import org.springframework.web.bind.annotation.*;

import java.util.List;

@RestController
@RequestMapping("api/members")
Expand All @@ -22,17 +20,31 @@ public class MemberController {

@PostMapping
public ResponseEntity<Void> create(@RequestBody MemberCreateRequest memberCreateRequest) {

memberService.create(memberCreateRequest);

return ResponseEntity.ok().build();
}

@GetMapping("/{id}")
public ResponseEntity<MemberResponse> findOne(@PathVariable("id") Long id) {

MemberResponse memberResponse = memberService.findOne(id);

return ResponseEntity.ok(memberResponse);
}

@GetMapping("/findAll")
public ResponseEntity<MemberResponse> findAll() {
List<MemberResponse> memberResponseList = memberService.findAll();
return ResponseEntity.ok((MemberResponse) memberResponseList);
}

@PutMapping("/{id}")
public ResponseEntity<Void> updateMember(@RequestBody MemberUpdateRequest memberUpdateRequest) {
memberService.updateMember(memberUpdateRequest);
return ResponseEntity.ok().build();
}

@DeleteMapping("/{id}")
public ResponseEntity<Void> deleteMember(@PathVariable("id") Long id) {
memberService.deleteMember(id);
return ResponseEntity.ok().build();
}
}
2 changes: 1 addition & 1 deletion src/main/java/com/example/jpa/member/domain/Member.java
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ public Member(String name) {
this.name = name;
}

void update(String name) {
public void update(String name) {
this.name = name;
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
package com.example.jpa.member.dto.request;

import com.example.jpa.member.domain.Member;
import lombok.Getter;
import lombok.Setter;

@Getter
public class MemberUpdateRequest {

private Long id;
private String name;
}
29 changes: 28 additions & 1 deletion src/main/java/com/example/jpa/member/service/MemberService.java
Original file line number Diff line number Diff line change
Expand Up @@ -2,10 +2,15 @@

import com.example.jpa.member.domain.Member;
import com.example.jpa.member.dto.request.MemberCreateRequest;
import com.example.jpa.member.dto.request.MemberUpdateRequest;
import com.example.jpa.member.dto.response.MemberResponse;
import com.example.jpa.repository.BoardRepository;
import com.example.jpa.repository.MemberRepository;

import java.util.List;
import java.util.NoSuchElementException;
import java.util.Optional;

import lombok.RequiredArgsConstructor;
import org.springframework.stereotype.Service;
import org.springframework.transaction.annotation.Transactional;
Expand All @@ -15,6 +20,7 @@
public class MemberService {

private final MemberRepository memberRepository;
private final BoardRepository boardRepository;

@Transactional
public void create(MemberCreateRequest memberCreateRequest) {
Expand All @@ -23,11 +29,32 @@ public void create(MemberCreateRequest memberCreateRequest) {

@Transactional(readOnly = true)
public MemberResponse findOne(Long id) {
Member member = checkMember(id);
return MemberResponse.from(member);
}

@Transactional(readOnly = true)
public Member checkMember(Long id) {
Member member = memberRepository.findById(id)
.orElseThrow(() -> new NoSuchElementException("멤버가 존재하지 않습니다."));
return member;
}

return MemberResponse.from(member);
@Transactional(readOnly = true)
public List<MemberResponse> findAll() {
List<Member> memberListTemp = memberRepository.findAll();
List<MemberResponse> memberResponseList = memberListTemp.stream().map(MemberResponse::from).toList();
return memberResponseList;
}

@Transactional
public void updateMember(MemberUpdateRequest memberUpdateRequest) {
Member member = checkMember(memberUpdateRequest.getId());
member.update(memberUpdateRequest.getName());
}

@Transactional(readOnly = true)
public void deleteMember(Long id) {
memberRepository.delete(checkMember(id));
}
}
Loading