Conversation
BaeJinho4028
left a comment
There was a problem hiding this comment.
5주차 과제 피드백입니다.
깔끔하게 잘 작성해주셨네요. 기대하고 있는만큼 잘 해주셨습니다.
아프신거 얼른 나으셨으면 좋겠습니다.
| public User jsonPage() { | ||
| User user = new User(); | ||
| user.setName("배진호"); | ||
| user.setAge(20); // ^_^ |
| public void setName(String name) { | ||
| this.name = name; | ||
| } | ||
|
|
||
| public void setAge(Number age) { | ||
| this.age = age; | ||
| } |
There was a problem hiding this comment.
setter의 단점을 알아보시면 좋을 것 같습니다.
(부제 : setter대신 생성자나 builder를 선호하는 이유?)
| @GetMapping("/hello2") | ||
| public String hello2() { | ||
| return "hello"; | ||
| @GetMapping("/json") |
There was a problem hiding this comment.
과제의 요구사항은 잘 지켜주셨습니다!
저라면 /introduce/html, /introdue/string, /introduce/json 이런식으로 일관되게 통일했을 것 같습니다.
일관된 코드 (=클린코드) 를 위해 참고해주시면 좋겠습니다.
| import org.springframework.web.bind.annotation.ResponseBody; | ||
|
|
||
| @Controller | ||
| public class HelloController { |
BaeJinho4028
left a comment
There was a problem hiding this comment.
6주차 과제도 수고하셨습니다.
몇가지 피드백 남겨놓았습니다.
현재 매우 잘하고 계십니다. 👍
| public void deleteMemberById(Long id) { | ||
| memberRepository.deleteById(id); | ||
| } | ||
| } No newline at end of file |
There was a problem hiding this comment.
https://velog.io/@junho5336/No-newline-at-end-of-file
참고하시면 좋을 것 같습니다.
There was a problem hiding this comment.
우왕... 이런 규칙도 있었군요ㅠ_ㅠ 이후 모든 코드에서 수정하였습니다~!
| @@ -0,0 +1,23 @@ | |||
| package com.example.bcsd; | |||
|
|
|||
| public class User { | |||
|
|
||
| import com.fasterxml.jackson.annotation.JsonIgnore; | ||
|
|
||
| public class Post { |
| public BoardService(BoardRepository boardRepository) { | ||
| this.boardRepository = boardRepository; | ||
| } |
There was a problem hiding this comment.
@requiredargsconstructor
롬복도 알아보시면 좋을 것 같아요
| import org.springframework.web.bind.annotation.ResponseBody; | ||
|
|
||
| @Controller | ||
| public class ArticleController { |
There was a problem hiding this comment.
Post라면 이것도 PostController가 맞지 않을까요?
There was a problem hiding this comment.
또한 패키지 구조도 기능별로 분리를 하는 것을 추천드립니다.
| return "posts"; | ||
| } | ||
|
|
||
| @GetMapping("/article") |
| public ResponseEntity<List<Post>> getArticles() { | ||
| List<Post> posts = postService.getAllPosts(); | ||
| if (posts.isEmpty()) { | ||
| return ResponseEntity.status(HttpStatus.NOT_FOUND).build(); |
There was a problem hiding this comment.
이런 검사는 서비스로직에서 예외로 던져주는게 좋을 것 같아요
| return post; | ||
| } | ||
|
|
||
| public boolean deletePost(Integer id) { |
There was a problem hiding this comment.
boolean보다는 null일경우 그냥 넘어가거나, 아니면 예외를 던지거나 둘중하나를 하는 것이 좋을 것 같습니다.
BaeJinho4028
left a comment
There was a problem hiding this comment.
이번 주차도 고생하셨습니다!
열심히 하신게 눈에 보여서 좋네요 👍👍
| } | ||
|
|
||
| dependencies { |
| public void deleteMemberById(Long id) { | ||
| memberRepository.deleteById(id); | ||
| } | ||
| } No newline at end of file |
| Optional<Member> existingMember = memberDao.findByEmail(member.getEmail()); | ||
| if (existingMember.isPresent() && !existingMember.get().getId().equals(member.getId())) { | ||
| throw new IllegalStateException("이미 사용 중인 이메일입니다."); | ||
| } |
There was a problem hiding this comment.
중복 체크 좋습니다. 👍
커스텀 예외도 한번 만들어보시면 좋을 것 같아요
| public class MemberService { | ||
| private final MemberRepository memberRepository; | ||
| private final MemberDao memberDao; | ||
|
|
||
| public MemberService(MemberRepository memberRepository) { | ||
| this.memberRepository = memberRepository; | ||
| public MemberService(MemberDao memberDao) { | ||
| this.memberDao = memberDao; | ||
| } | ||
|
|
||
| public List<Member> findAllMembers() { | ||
| return memberRepository.findAll(); | ||
| return memberDao.findAll(); | ||
| } | ||
|
|
||
| public Optional<Member> findMemberById(Long id) { | ||
| return memberRepository.findById(id); | ||
| return memberDao.findById(id); | ||
| } | ||
|
|
||
| public Optional<Member> findMemberByEmail(String email) { | ||
| return memberRepository.findByEmail(email); | ||
| return memberDao.findByEmail(email); | ||
| } |
There was a problem hiding this comment.
클래스 위에
@Transactional(readOnly = true)
를 붙이면 좋습니다.
| public String posts(Model model, HttpServletRequest request) { | ||
| String boardId = request.getParameter("boardId"); | ||
| List<Article> articles; | ||
| if (boardId != null && !boardId.isEmpty()) { | ||
| articles = articleService.findArticlesByBoardId(Long.valueOf(boardId)); | ||
| model.addAttribute("boardId", boardId); | ||
| } else { | ||
| articles = articleService.findAllArticles(); | ||
| } | ||
| model.addAttribute("posts", articles); | ||
| return "posts"; | ||
| } |
There was a problem hiding this comment.
HttpServletRequest를 쓰지말고 request dto를 만들고 @Valid 로 이런거 검증하는 방법을 찾아보시면 좋을 것 같습니다. 고생 많이 하셨네요 ㅎㅎ :)
BaeJinho4028
left a comment
There was a problem hiding this comment.
이번 주차 과제도 고생하셨습니다.
항상 코드가 깔끔해서 리뷰할 맛이 나네요.
잘하고 계십니다. 👍👍
| public BoardController(BoardService boardService) { | ||
| this.boardService = boardService; | ||
| } |
There was a problem hiding this comment.
@requiredargsconstructor
보고서에 매번 쓰시는데 활용해보셔도 좋지 않을까요?
| boardService.deleteBoardById(id); | ||
| return ResponseEntity.noContent().build(); | ||
| } | ||
| } No newline at end of file |
| return ResponseEntity.status(HttpStatus.INTERNAL_SERVER_ERROR).body(response); | ||
| } | ||
|
|
||
| private Map<String, Object> createErrorResponse(String message, int status) { |
There was a problem hiding this comment.
커스텀 예외 좋습니다.👍
굳이 Map이 아니라 클래스를 하나 만들어주셔도 좋을 것 같아요
| @@ -0,0 +1,7 @@ | |||
| package com.example.bcsd.exception; | |||
|
|
|||
| public class InvalidRequestException extends RuntimeException { | |||
There was a problem hiding this comment.
Exception들을 더 찾아보시면 좋을 것 같아요. IllegalArgumentException 같은 예외를 상속하는것이 더 좋을 것 같아요
|
|
||
| public Optional<Member> findMemberById(Long id) { | ||
| return memberDao.findById(id); | ||
| public Member findMemberById(Long id) { |
There was a problem hiding this comment.
사실 DB용 명령어는 Service 계층에서 안나타내는것이 좋습니다.
find -> get
save -> create
이런식으로 쓰시는게 좋을 것 같습니다.
| if (article.getTitle() == null || article.getContent() == null || | ||
| article.getMemberId() == null || article.getBoardId() == null) { |
BaeJinho4028
left a comment
There was a problem hiding this comment.
가면 갈수록 코드가 깔끔해지고 있군요. 고생하셨습니다!
졸작 전시회 때문에 정신이 없네요
| @PostMapping("/board") | ||
| public ResponseEntity<Board> createBoard(@RequestBody Board board) { | ||
| Board savedBoard = boardService.saveBoard(board); | ||
| public ResponseEntity<Board> createBoard(@Valid @RequestBody BoardRequestDto boardDto) { |
| } | ||
|
|
||
| @GetMapping("/posts") | ||
| public String posts(Model model, HttpServletRequest request) { |
| public String posts(Model model, HttpServletRequest request) { | ||
| String boardId = request.getParameter("boardId"); | ||
| List<Article> articles; | ||
| if (boardId != null && !boardId.isEmpty()) { |
There was a problem hiding this comment.
StringUtils.isEmpty()도 있습니다.
근데 생각해보면 공백문자열 체크를 위해서는 isBlank()를 쓰시는게 좋습니다 :)
BaeJinho4028
left a comment
There was a problem hiding this comment.
화이팅입니다. 알아서도 잘 하시겠지만, 방학때 무언가를 꾸준히 공부하는것이 중요하다고 생각합니다. 목표같은것을 잡고 하나 공부해보시면 좋을 것 같습니다.
| if (boardId != null && !boardId.isEmpty()) { | ||
| articles = articleService.getArticlesByBoardId(Long.valueOf(boardId)); | ||
| model.addAttribute("boardId", boardId); | ||
| public String posts(Model model, @Valid @ModelAttribute ArticleSearchRequestDto searchDto) { |
There was a problem hiding this comment.
@ModelAttribute를 사용하면 요청 쿼리 파라미터를 자동으로 DTO 객체에 바인딩해주기 때문입니다!
| public ArticleResponseDto() { | ||
| } | ||
|
|
||
| public ArticleResponseDto(Long id, Long memberId, Long boardId, String boardName, | ||
| String title, String content, LocalDateTime createdAt, LocalDateTime updatedAt) { | ||
| this.id = id; | ||
| this.memberId = memberId; | ||
| this.boardId = boardId; | ||
| this.boardName = boardName; | ||
| this.title = title; | ||
| this.content = content; | ||
| this.createdAt = createdAt; | ||
| this.updatedAt = updatedAt; | ||
| } | ||
|
|
||
| public Long getId() { | ||
| return id; | ||
| } | ||
|
|
||
| public void setId(Long id) { | ||
| this.id = id; | ||
| } | ||
|
|
||
| public Long getMemberId() { | ||
| return memberId; | ||
| } | ||
|
|
||
| public void setMemberId(Long memberId) { | ||
| this.memberId = memberId; | ||
| } | ||
|
|
||
| public Long getBoardId() { | ||
| return boardId; | ||
| } | ||
|
|
||
| public void setBoardId(Long boardId) { | ||
| this.boardId = boardId; | ||
| } | ||
|
|
||
| public String getBoardName() { | ||
| return boardName; | ||
| } | ||
|
|
||
| public void setBoardName(String boardName) { | ||
| this.boardName = boardName; | ||
| } | ||
|
|
||
| public String getTitle() { | ||
| return title; | ||
| } | ||
|
|
||
| public void setTitle(String title) { | ||
| this.title = title; | ||
| } | ||
|
|
||
| public String getContent() { | ||
| return content; | ||
| } | ||
|
|
||
| public void setContent(String content) { | ||
| this.content = content; | ||
| } | ||
|
|
||
| public LocalDateTime getCreatedAt() { | ||
| return createdAt; | ||
| } | ||
|
|
||
| public void setCreatedAt(LocalDateTime createdAt) { | ||
| this.createdAt = createdAt; | ||
| } | ||
|
|
||
| public LocalDateTime getUpdatedAt() { | ||
| return updatedAt; | ||
| } | ||
|
|
||
| public void setUpdatedAt(LocalDateTime updatedAt) { | ||
| this.updatedAt = updatedAt; | ||
| } |
There was a problem hiding this comment.
lombok을 써보시는건 어떨까요?
setter가 과연 필요할까요?
| private Long id; | ||
| private Long memberId; | ||
| private Long boardId; | ||
| private String boardName; | ||
| private String title; | ||
| private String content; | ||
| private LocalDateTime createdAt; | ||
| private LocalDateTime updatedAt; |
There was a problem hiding this comment.
@Valid 의 검증 애너테이션들을 활용해도 좋을 것 같습니다.
| article.getBoard() != null ? article.getBoard().getId() : null, | ||
| article.getBoard() != null ? article.getBoard().getName() : null, |
|
|
||
| Optional<Board> existingBoard = boardDao.findByName(board.getName()); | ||
| Optional<Board> existingBoard = boardRepository.findByName(board.getName()); | ||
| if (existingBoard.isPresent() && (board.getId() == null || !existingBoard.get().getId().equals(board.getId()))) { |
|
|
||
| if (!existingBoardEntity.getName().equals(boardDto.getName())) { | ||
| Optional<Board> duplicateNameBoard = boardDao.findByName(boardDto.getName()); | ||
| Optional<Board> duplicateNameBoard = boardRepository.findByName(boardDto.getName()); |
There was a problem hiding this comment.
exists 라는 존재하는지를 boolean을 반환해주는 메서드도 있습니다.
BaeJinho4028
left a comment
There was a problem hiding this comment.
이번주차 과제도 고생하셨습니다.👍
aws배포 후 회고프로젝트를 진행하기 때문에 프로젝트 아이디어를 한번 고민해보시면 좋을 것 같습니다.
| public static ArticleResponseDto from(Article article) { | ||
| Board board = article.getBoard(); | ||
| return new ArticleResponseDto( | ||
| article.getId(), | ||
| article.getMemberId(), | ||
| board != null ? board.getId() : null, | ||
| board != null ? board.getName() : null, | ||
| article.getTitle(), | ||
| article.getContent(), | ||
| article.getCreatedAt(), | ||
| article.getUpdatedAt() | ||
| ); |
| @NotNull | ||
| @Min(1) |
There was a problem hiding this comment.
Response를 검증하는 이유가 있을까요?
검증이 필요하다하더라도 과연 동작할까요?
| @Setter | ||
| @NoArgsConstructor | ||
| @AllArgsConstructor | ||
| @ToString(exclude = {"password", "confirmPassword"}) |
| if (!member.getPassword().equals(password)) { | ||
| throw new InvalidRequestException("잘못된 비밀번호입니다."); | ||
| } |
There was a problem hiding this comment.
비밀번호를 평문으로 저장하거나, 평문으로 비교하면 안됩니다 🫠
BCryptPasswordEncoder 참고해보시죠
| Model model) { | ||
| try { | ||
| Member member = authService.login(email, password); | ||
| session.setAttribute("member", member); |
No description provided.