-
Notifications
You must be signed in to change notification settings - Fork 27
[이하원_BackEnd] 10주차 과제 제출합니다. #5
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
kih1015
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.
고생하셨습니다!
코멘트 확인해주세요
| @RestController | ||
| @RequestMapping("/article") | ||
| public class ArticleController { |
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 prefix를 적용해주셨네요.
프로젝트가 커질 경우 이렇게 prefix 단위로 컨트롤러를 분리해주면 관리가 용이하겠죠
| } | ||
| } No newline at end of file |
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.
EOF에 대해 알아보시면 좋을 것 같아요
| if (content != null) { | ||
| long id = articleId++; | ||
| articles.put(id, content); | ||
| response.put("message", "Article created"); | ||
| response.put("id", id); | ||
| return new ResponseEntity<>(response, HttpStatus.CREATED); | ||
| } else { | ||
| response.put("message", "Content is missing"); | ||
| return new ResponseEntity<>(response, HttpStatus.BAD_REQUEST); | ||
| } |
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.
현재 분기가 너무 복잡한데 읽기 좋은 코드를 위해 else 문 없이 조기 리턴으로 코드를 개선하는게 가능해 보여요
| if (article != null) { | ||
| response.put("content", article); | ||
| return new ResponseEntity<>(response, HttpStatus.OK); | ||
| } else { | ||
| response.put("message", "Article not found"); | ||
| return new ResponseEntity<>(response, HttpStatus.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.
여기도 마찬가지입니다
| if (articles.containsKey(id)) { | ||
| if (content != null) { | ||
| articles.put(id, content); | ||
| response.put("message", "Article updated"); | ||
| return new ResponseEntity<>(response, HttpStatus.OK); | ||
| } else { | ||
| response.put("message", "Content is missing"); | ||
| return new ResponseEntity<>(response, HttpStatus.BAD_REQUEST); | ||
| } | ||
| } else { | ||
| response.put("message", "Article not found"); | ||
| return new ResponseEntity<>(response, HttpStatus.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.
마찬가지로 중첩된 조건문은 피해주시는게 좋아요.
cf) 중첩 조건문 리팩토링
| response.put("message", "Content is missing"); | ||
| return new ResponseEntity<>(response, HttpStatus.BAD_REQUEST); |
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.
응답 엔티티를 더 가독성 있게 만들 수 있어요
| </body> | ||
| </html> No newline at end of file |
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.
여기도 EOF!
|
6주차 과제에 PR하라는 말은 없었지만 코드가 너무 많아 보고서에 넣긴 힘들것같아서 이곳으로 PR합니다. |
|
무슨 문제인지는 모르겠지만 force push으로 올렸습니다 |
kih1015
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.
고생많으셨습니다!
마지막까지 화이팅
| @RequestMapping("/article") | ||
| @RequestMapping("/articles") |
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.
제 기억에는 원래 맴버, 게시판, 게시글에 대한 모델을 만드는게 과제여서 그때는 article로 사용했지만
과제 요구사항에서 /articles 로 요청을 보내도록 되어있었기에 요구사항에 맞춰서 바꿨습니다
| private Article(Builder builder) { | ||
| id = builder.id; | ||
| userId = builder.userId; | ||
| boardId = builder.boardId; | ||
| postDate = builder.postDate; | ||
| title = builder.title; | ||
| content = builder.content; | ||
| putDate = builder.putDate; | ||
| } | ||
|
|
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.
기본 생성자를 private으로 설정하신 이유가 궁금해요!
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.
빌더관련 참고한 블로그에서 private로 설정해서 저도 따라서 작성하였습니다
아마 빌더를 사용하는걸 강제하고 실수가 생기기 쉬운 기본 생성자를 private로 설정한것 같습니다
| public void setTitle(String title) { | ||
| this.title = title; | ||
| } | ||
|
|
||
| public void setContent(String content) { | ||
| this.content = content; | ||
| } | ||
|
|
||
| public void setPutDate(String putDate) { | ||
| this.putDate = putDate; | ||
| } |
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.
요구사항에서 세터가 꼭 필요한가요?
게시글의 수정이 요구사항에 포함되어 있다면, 목적에 맞는 수정을 위한 메서드(예: update(String, String, String))를 만들어 활용하는 편이 좋아보여요.
사용 목적과 의미가 명확하지않고 내부 값에 대한 접근을 무방비하게 노출하는 세터는 사용을 지양하는 것이 좋다고 생각합니다.
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.
네 이부분은 이전 과제에서 자료구조를 사용할때 사용된 코드여서
저도 해당 과제에서는 없는것이 맞다고 생각합니다
| private final String postDate; | ||
| private String title; | ||
| private String content; | ||
| private String putDate; |
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.
postDate, putDate 보다는
createdAt, modifiedAt
위와 같은 필드명이 더 적절하다고 생각합니다.
|
|
||
| public Builder(long id, long uId, long bId, String pD) { | ||
| this.id = 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.
매개변수명도 축약하기보다 조금 길어지더라도 의미를 명확히 알 수 있도록 작성하면 좋을 것 같아요.
| public static class Builder { | ||
| private final long id; | ||
| private final long userId; | ||
| private final long boardId; | ||
| private final String postDate; | ||
| private String title; | ||
| private String content; | ||
| private String putDate = ""; |
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.
빌더 직접 구현해보신 것 좋아요!
| public ResponseEntity<Article> put(@PathVariable long id, | ||
| @RequestBody Map<String, String> article) { | ||
| if (!articleService.validArticle(id)) { | ||
| return ResponseEntity.notFound().build(); | ||
| } | ||
| String password = article.get("password"); | ||
| if(!memberService.findById(articleService.findById(id).getUserId()).getPassword().equals(password)){ | ||
| return ResponseEntity.badRequest().build(); | ||
| } | ||
| String title = article.get("title"); | ||
| String content = article.get("content"); | ||
| Article updated = articleService.update(id, title, content); | ||
| return ResponseEntity.ok(updated); | ||
| } |
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.
분기처리가 간단해져서 코드 흐름이 눈에 잘들어오네요.
| @Repository | ||
| public class ArticleRepository { | ||
| private JdbcTemplate jdbcTemplate; | ||
|
|
||
| ArticleRepository(JdbcTemplate jdbcTemplate) { | ||
| this.jdbcTemplate = jdbcTemplate; | ||
| } |
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.
굿굿 전체적으로 잘해주셨네요.
|
|
||
| public Article findById(long id) { | ||
| return articleRepository.findById(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.
읽기 전용 트랜잭션의 경우
@Transactional(readonly = True) 성능에 도움이 될 수 있습니다.
공부해보시는 것 추천드려요.
| Article created = articleService.save(userId, boardId, title, content); | ||
| return ResponseEntity.status(HttpStatus.CREATED).body(created); |
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.
ReponseEntity.created() 메서드 사용을 추천합니다.
|
8주차 과제 수행하였습니다. |
kih1015
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.
고생많으셨습니다!
코멘트 확인해주세요.
| private final String postDate; | ||
| private final String createdAt; | ||
| private String title; | ||
| private String content; | ||
| private String putDate; | ||
| private String modifiedAt; | ||
|
|
||
| private Article(Builder builder) { | ||
| id = builder.id; | ||
| userId = builder.userId; | ||
| boardId = builder.boardId; | ||
| postDate = builder.postDate; | ||
| createdAt = builder.createdAt; | ||
| title = builder.title; | ||
| content = builder.content; | ||
| putDate = builder.putDate; | ||
| modifiedAt = builder.modifiedAt; |
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.
👍
| public String getPutDate() { | ||
| return putDate; | ||
| } | ||
|
|
||
| public void setTitle(String title) { | ||
| this.title = title; | ||
| } | ||
|
|
||
| public void setContent(String content) { | ||
| this.content = content; | ||
| } | ||
|
|
||
| public void setPutDate(String putDate) { | ||
| this.putDate = putDate; | ||
| public String getModifiedAt() { | ||
| return modifiedAt; |
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.
클래스를 단순히 데이터 덩어리로 바라보지 않고 완전한 모듈로 바라보는 객체지향적 설계라면
이렇게 내부 값에 대한 노출을 최대한 자제하면 좋을 것 같아요.
| try { | ||
| memberService.validId(userId); | ||
| boardService.validId(boardId); | ||
| } | ||
| catch (RuntimeException e){ | ||
| throw new ReferencedEntityNotFoundException("유효하지 않은 게시판 또는 사용자 입니다."); |
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.
커스텀 예외를 만들어 처리해 준 것 좋네요.
다만 하위 모듈(서비스, 리파지토리)에서 직접 커스텀 예외를 던져도 좋지 않을까요?
컨트롤러에서 예외를 다시 잡아서 다시 던지는 이유가 궁금하네요.
| return ResponseEntity.status(HttpStatus.CREATED).body(created); | ||
| return ResponseEntity.created(URI.create("/articles/" + created.getId())).build(); |
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.
훌륭합니다!
| try{ | ||
| Article article = articleService.findById(id); | ||
| return ResponseEntity.ok(article); | ||
| } catch (RuntimeException e) { | ||
| throw new EntityNotFoundException("게시물을 찾을 수 없습니다"); |
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.
여기서도 하위모듈에서 직접 EntityNotFoundException을 던져도 되지 않았을까요
| @Transactional(readOnly=true) | ||
| public Article findById(long id) { | ||
| return articleRepository.findById(id); | ||
| } | ||
|
|
||
| @Transactional(readOnly=true) |
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.
@Transactional(readOnly=true) 활용해주셨네요.
| try{ | ||
| Board board = boardService.findById(id); | ||
| return ResponseEntity.ok(board); | ||
| } catch (RuntimeException e) { | ||
| throw new EntityNotFoundException("게시판을 찾을 수 없습니다."); | ||
| } |
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.
여기도 마찬가지! 하위모듈에서 직접 예외를 던져도 될 것 같아요.
| @RestControllerAdvice | ||
| public class DBExceptionHandler { | ||
|
|
||
| @ExceptionHandler(EmailAlreadyExistsException.class) | ||
| public ResponseEntity<String> handleEmailAlreadyExistsException(EmailAlreadyExistsException e){ | ||
| return ResponseEntity.status(e.getStatusCode()).body(e.getMessage()); | ||
| } | ||
|
|
||
| @ExceptionHandler(EntityHasArticleException.class) | ||
| public ResponseEntity<String> handleEntityHasArticleException(EntityHasArticleException e){ | ||
| return ResponseEntity.status(e.getStatusCode()).body(e.getMessage()); | ||
| } |
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.
잘 작성해주셨네요.
예외 처리 책임을 분리하니 정말 깔끔하죠 try-catch 도배할 필요도 없구요.
| try { | ||
| String email = article.get("email"); | ||
| Member updated = memberService.update(id, email); | ||
| return ResponseEntity.ok(updated); | ||
| } | ||
| catch (RuntimeException e){ | ||
| throw new EmailAlreadyExistsException("중복 이메일 입니다."); | ||
| } |
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.
try-catch 사용하지 않을 방법이 있지 않을까요?
kih1015
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.
고생하셨습니다!
거의 다 왔네요 끝까지 화이팅!
| this.content = content; | ||
| this.createdAt = LocalDateTime.now(); |
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.
@CreatedDate 어노테이션에 대해 알아보는 것도 좋을 것 같아요
과제 수준에서는 변경하지 않아도 괜찮을 것 같습니다!
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.
네 더불어서
@LastModifiedDate 등도 과제하면서 찾아봤는데 해당 과제에서는 기존에 쓰던 기능으로 해도 무관할것같아 사용하지 않았습니다
| if(!memberService.findById(articleService.findById(id).getUserId()).getPassword().equals(password)){ | ||
| if (!memberService.findById(articleService.findById(id).getMemberId()).getPassword().equals(password)) { |
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.
임시 변수를 활용한다면 읽기 편할 것 같아요
Long userId = articleService
.findById(id)
.getUserId();
String memberPassword = memberService
.findById(userId)
.getPassword();
if (!memberPassword.equals(password)) {
// 처리 로직
}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.
네 다음 과제부터 활용해보겠습니다
| String now = LocalDateTime.now().format(formatter); | ||
| return articleRepository.save(userId, boardId, now, title, content); | ||
| public Article save(long memberId, long boardId, String title, String content) { | ||
| Board board = boardRepository.findById(boardId).get(); |
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.
null 반환한다면 어쩌죠?
null을 핸들링 하기 위해 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.
맞습니다 과제할때 미처 생각하지 못한 문제였던것 같습니다
| @OneToMany(mappedBy = "board", cascade = CascadeType.ALL, orphanRemoval = true) | ||
| 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.
👍
| } catch (RuntimeException e) { | ||
| throw new ReferencedEntityNotFoundException("유효하지 않은 게시판 또는 사용자 입니다."); | ||
| } |
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.
service에서 ReferencedEntityNotFoundException 예외를 던져도 되지 않았을까요?
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.
그리고 제가 저번주 수요일부터 미국에와있고 7월 4일까지 해외에 있어서 이번 11주차과제를 수행 못하여 미리 말씀드립니다
해당 과제는 12주차 과제때 함께 구현하여 제출하도록 하겠습니다
과제를 진행할때 구현한 기능들은
해당 과제 부분을 설명할때 링크로 첨부한 내용들을 최대한 넣어보려 했습니다 (ResponseBody, ResponseEntity 등)
느낀점은 보고서에 작성하였습니다