feat: YSL-24 오늘의 다짐 CRUD 기능 추가 #16
Conversation
|
|
||
| import lombok.Getter; | ||
|
|
||
| public class BusinessExceptionHandler extends RuntimeException{ |
There was a problem hiding this comment.
BusinessExceptionHandler 라는 명칭이라고 하면 Exception이 발생했을때 처리를 해주는 역할 이라고 생각이드는데요
다른 명칭을 쓰는게 좋아보입니다.
There was a problem hiding this comment.
제가 이해한 바로는 TodoExceptionHandler보다는 TodoException 같은게 더 맞다는 의미같아요...
TodoHandler가 무엇을 뜻하는지 오히려 더 모르겠습니다 🥲
그리고 TodoException 같이 하나로 묶는 것보단
NotFoundException이 더 직관적이지 않을까요?
직접 오류가 나오는 케이스에 대해서 API 요청을 해보고, 응답을 확인해보세욥!
ID_NOT_FOUND나 ID_DELETE 모두 자원이 없는 404의 상황이라고 저는 판단이 되는데요.
작성하신 코드가 정상적으로 404 응답이 나오나요?
[ @ControllerAdvice ]에 대해서 헷갈린다면 질문 주세요!
src/main/java/org/idiot/yesslave/todo/application/todoService.java
Outdated
Show resolved
Hide resolved
| @Service | ||
| @RequiredArgsConstructor | ||
| @Transactional | ||
| public class todoService { |
There was a problem hiding this comment.
클래스명 시작은 대문자로 해주는것이 기본 자바 컨벤션 입니다 😃!
관련글: 2.5 클래스/인터페이스 이름에 대문자 카멜표기법 적용
https://naver.github.io/hackday-conventions-java/#class-interface-lower-camelcase
|
|
||
| import lombok.Getter; | ||
|
|
||
| public class BusinessExceptionHandler extends RuntimeException{ |
There was a problem hiding this comment.
메소드나 클래스를 열 때, 시작 중괄호 "{" 전에 하나의 공백이 있는 것이 좋을 것 같습니다.
관련글: 8.3 중괄호의 시작 전, 종료 후에 공백 삽입
https://naver.github.io/hackday-conventions-java/#space-around-brace
| public class todoService { | ||
| private final todoRepository todoRepository ; | ||
|
|
||
| // 저장 |
There was a problem hiding this comment.
개인적으로, 이미 메서드 이름 자체가 해당 메서드가 무엇을 의미하는지 설명이 잘 되기 때문에 이러한 주석은 불필요하지 않을까? 싶습닏다.
|
|
||
| // 삭제 상태 확인 | ||
| private static void deleteStatus(todo todo) { | ||
| if (todo.isDelete()) throw new BusinessExceptionHandler(errorCode.ID_DELETE); |
There was a problem hiding this comment.
아주 간단한 if문이라도, 서로 간의 원할한 협업을 위해 컨벤션을 참고해보면 좋을 것 같아요!
관련글 : 5.4. 조건/반복문에 중괄호 필수 사용
https://naver.github.io/hackday-conventions-java/#space-around-comment
| private final todoService todoService; | ||
|
|
||
| @PostMapping | ||
| public ResponseEntity todoSave(@RequestBody saveDto saveDto) { |
There was a problem hiding this comment.
public과 ResponseEntity 사이에 공백이 두개인 것 같아요!
There was a problem hiding this comment.
두 개의 공백까지 찾아내시다니 정말 대단하세요
수정했습니다
| private LocalDateTime registerDate; | ||
|
|
||
| @Column(name = "UPDATE_DATE", nullable = true) | ||
| private LocalDateTime updateDate; |
There was a problem hiding this comment.
생성일자나 업데이트 일자 같은거는 만들어진 Auditing을 이용하면 좋을 것 같습니다.
|
|
||
| @Getter | ||
| @NoArgsConstructor(access = AccessLevel.PROTECTED) | ||
| // cannot deserialize from Object value (no delegate- or property-based Creator)에러로 인해 기본 생성자 필요 |
There was a problem hiding this comment.
트러블 슈팅에 관한 내용은 주석에 남기는 것보단 별도 글을 남겨서 공유를 해주시면 더 좋을 것 같아요.
매번 해결법에 대한 내용을 남기게 되면, 실제 코드보다 이런 내용이 더 많아질 것 같기 때문입니닫.
There was a problem hiding this comment.
처음 이 주석 작성할 때 이런 내용을 작성해도 괜찮은가 생각했었는데 역시 안 쓰는 게 좋은 거였네요.
주석 제거했습니다
| .build()); | ||
|
|
||
| //then | ||
| mvc.perform(delete(BASE_URL + "/1")) |
There was a problem hiding this comment.
mvc.perform(delete(BASE_URL + "/{id}", 1L))
...또는
//given
final Long id = 1L;
...
mvc.perform(delete(BASE_URL + "/{id}", id))
...이런식으로도 작성 가능합니다.
There was a problem hiding this comment.
이렇게도 작성을 할 수 있군요 첫 번째 방법으로 수정했습니다
| errorCode(String message) { | ||
| this.message = message; | ||
| } | ||
| } |
There was a problem hiding this comment.
깃헙에서 이런 마크는 왜 나오는걸까요?
관련글 한 번 봐주세욥
There was a problem hiding this comment.
항상 거슬린다고 생각했었는데 이번에 첨부해 주신 글을 보고 인텔리제이 설정을 바꿔서 해결했습니다
|
|
||
| @EnableJpaAuditing |
There was a problem hiding this comment.
요거 JpaConfig에 작성 되어 있던 내용인데 앞으로 꺼내오신 이유가 있으실까요?
There was a problem hiding this comment.
Auditing 사용하면서 오류가 발생해서 앞으로 꺼냈는데 다시 제거하고 확인해 보니까 정상적으로 동작하네요
코드를 작성하면서 뭔가 실수가 있었나 봐요. 지금은 지웠습니다
|
|
||
| import lombok.Getter; | ||
|
|
||
| public class BusinessExceptionHandler extends RuntimeException{ |
There was a problem hiding this comment.
제가 이해한 바로는 TodoExceptionHandler보다는 TodoException 같은게 더 맞다는 의미같아요...
TodoHandler가 무엇을 뜻하는지 오히려 더 모르겠습니다 🥲
그리고 TodoException 같이 하나로 묶는 것보단
NotFoundException이 더 직관적이지 않을까요?
직접 오류가 나오는 케이스에 대해서 API 요청을 해보고, 응답을 확인해보세욥!
ID_NOT_FOUND나 ID_DELETE 모두 자원이 없는 404의 상황이라고 저는 판단이 되는데요.
작성하신 코드가 정상적으로 404 응답이 나오나요?
[ @ControllerAdvice ]에 대해서 헷갈린다면 질문 주세요!
| } | ||
| } | ||
|
|
||
| public todo existId(Long id) { |
There was a problem hiding this comment.
@transactional에 readOnly 라는 옵션이 있습니다.
요거 한 번 그냥 @transactional이랑 readOnly = true 주는거랑 무슨 차이가 있는지 한 번 봐보시면 좋을 것 같아요
| this.todo = todo; | ||
| } | ||
| public void delete (boolean delete) { | ||
| this.delete = delete; |
There was a problem hiding this comment.
delete 메서드는 "삭제"에 대한 상태를 "true"로만 바꿔주는 역할만을 할 것 같습니다.
외부에서 false를 받아 처리를 하는 경우가 있다면 [ delete ] 라는 메서드 명이 잘못 되었다고 판단이 드는데요.
만약 "true"로만 바꿔주는 역할이라함은, 굳이 [ boolean delete ] 값을 받을 필요는 없을 것 같아요.
외부에 의한 요인으로 내부 객체 상태가 바뀌는 경우가 있지만,
명확한 경우에는 내부에서 직접 객체 상태를 변경해주는게 더 좋지 않을까 싶습니다.
There was a problem hiding this comment.
객체 상태(delete 여부)는 객체 자신(todo)이 바꿔야 된다고 생각을 하는 편입니다. 이래야 더 안전하고 좀 더 객체지향적인 코드를 작성을 한다고 저는 생각을 하고 있거든요 😂
저의 경우는 해당 코드를 아래와 같이 작성할 것 같아요
public void delete() {
this.delete = true;
}지금 주철님의 코드 경우에는 service계층에서 delete를 통해서 true를 넘겨주지만,
혹시 누가 몰래 false를 넣어주는 코드를 실수로 넣었다고 하면, 이를 또 하나씩 찾아야 하잖아요.
다행인건 해당 메서드는 boolean으로 넣어질 수 있는 값이 한정적이지만, String 같은걸로, 혹은 값이 다양한 enum 형식이였다면, 충분히 상태가 잘못될 수 있는 상황을 열어주는 것이라고 봅니다.
외부라는 건, 다른 클래스가 될 수도 있을 것 같네요.
혹시 질문에 답변이 되었을까요?
|
|
||
| @SpringBootTest | ||
| @AutoConfigureMockMvc | ||
| public class TodoControllerTest { |
There was a problem hiding this comment.
테스트 코드 작성 경험이 적으신 와중에 이렇게 짜주셔서 감사합니다!
제가 생각했을 때 테스트코드는 [ 성공 ] 보다는 [ 실패 ]에 대한 경우를 잡는게 더 중요하다고 생각하는데요.
주철님이 짜주신 테스트 코드는 전반적으로 [ 성공 ] 의 경우만 작성이 된 것 같아요.
아직 어색하시겠지만, 제가 곧 올리는 PR에서 "실패"의 경우는 어떻게 처리를 했는지 한 번 봐보시면 좋을 것 같습니다.
| @Service | ||
| @RequiredArgsConstructor | ||
| public class TodoService { | ||
| private final TodoRepository todoRepository ; |
There was a problem hiding this comment.
변수명과 세미콜론 사이에 공백이 들어가 있습니다 🤤
There was a problem hiding this comment.
공백 제거했습니다
필요 없는 공백이 많네요. 코드를 다시 확인하는 습관을 기르겠습니다
| import org.hibernate.annotations.Comment; | ||
| import org.idiot.yesslave.global.jpa.AuditInformation; | ||
|
|
||
| import javax.persistence.*; |
There was a problem hiding this comment.
와일드 카드가 사용이 되었는데, 한 번 아래 참고글 봐보시면 좋을 것 같아용
There was a problem hiding this comment.
와일드카드가 컴파일 속도를 느리게 한다는 것은 알고 있었는데 인텔리제이 설정에서 바꿀 수 있는 건 처음 알았습니다.
지금은 수정했습니다
| private boolean todoCheck = false; | ||
|
|
||
| @Column(name = "DELETE", nullable = false) | ||
| @Comment("delete는 삭제 여부를 체크하는 부분입니다.") |
There was a problem hiding this comment.
실제 DB 설계하실 때에도 코멘트에 문장으로 작성하시는 편일까요..? 🤔
There was a problem hiding this comment.
주석처럼 생각하고 문장으로 작성했습니다.
진주님 코드 보고 저도 erdcloud에 있는 코멘트로 바꿨습니다!
| import org.idiot.yesslave.todo.domain.todo; | ||
| import org.springframework.data.jpa.repository.JpaRepository; | ||
|
|
||
| public interface TodoRepository extends JpaRepository<todo,Long>{ |
There was a problem hiding this comment.
JpaRepository<todo,Long>{
중괄호 시작 전에 공백이 하나 있으면 좋을 것 같습니다.
There was a problem hiding this comment.
항상 이런 실수를 못 보고 지나가네요 수정하겠습니다
| main: | ||
| allow-bean-definition-overriding: true |
There was a problem hiding this comment.
해당 옵션이 어떠한 역할을 하나요?
어떠한 경우 때문에 작성이 된건지 궁금합니다.
There was a problem hiding this comment.
깃허브 빌드에서 발생하는 테스트 코드 에러를 없애려고 사용했는데 지금 보니까 제 테스트 자체에도 문제가 있어서 수정 중입니다.
There was a problem hiding this comment.
main:
allow-bean-definition-overriding: true
은 지금 삭제했습니다.
테스트 코드는 좀 더 수정하고 올리겠습니다!
| @Builder | ||
| public Todo(String todo) { | ||
| this.todo = todo; | ||
| } |
There was a problem hiding this comment.
저는 이런식으로 안써봐서 잘 모르는데, 어떠한 이점이 있는지 궁금해요. 혹시 알려주실 수 있으실까요? 👀
There was a problem hiding this comment.
제가 처음 스프링 입문할 때 이런 방식으로 시작해서 사용한 것도 있고, 여기서 입력할 게 1개만 있는 경우에는 생성자를 만들어서 사용하는 게 더 깔끔할 것 같아서 사용했어요.
혹시 좋은 방식이 있으면 알려주실 수 있으실까요?
| @Comment("실행여부") | ||
| private boolean todoCheck = false; | ||
|
|
||
| @Column(name = "DELETE", nullable = false) |
There was a problem hiding this comment.
DELETE가 혹시 MySQL의 예약어라는 점... 아셨을까요!? 👀
좀 더 안전한 칼럼명을 선택하는 것이 좋아보일 것 같아요 👍🏻
There was a problem hiding this comment.
알고는 있었는데 이런 식으로 같이 사용하면 까먹고 사용하다 오류가 발생하면 그때 생각나더라고요….
이것도 수정하겠습니다
| public void delete () { | ||
| delete = true; | ||
| } |
There was a problem hiding this comment.
delete와 파람인자를 받는 소괄호 사이의 공백이 보이는데, 요 부분 한 번 봐주시면 좋을 것 같습니다
| return ResponseEntity.ok("Todo save successfully"); | ||
| } | ||
|
|
||
| @PatchMapping("/{id}") |
There was a problem hiding this comment.
저는 Patch형식보다는 Put을 주로 더 선호하는 편인데, 이유는 Patch의 경우 바꾸고자 하는 내용만 처리해줘야하는 "별도 로직"을 작성을 해야한다는 점 때문이에요.
주철님의 경우, UpdateDto 내부에 todo 밖에 없기 때문에 크게 문제는 되지 않을 것 같지만, 한 번 아래 글 읽어보시면 좋을 것 같아용
| @PostMapping("/{id}") | ||
| public boolean changeCheck(@PathVariable Long id) { | ||
| return todoService.changeCheck(id); | ||
| } |
There was a problem hiding this comment.
Http Method의 Post의 역할은 서버에 자원을 저장으로 알고 있는데,
해당 행위는 Post가 맞는걸까요? 한 번 고민 해보시면 좋을 것 같습니다
| main: | ||
| allow-bean-definition-overriding: true |
| @@ -29,7 +29,7 @@ | |||
| import static org.springframework.test.web.servlet.result.MockMvcResultHandlers.print; | |||
|
|
|||
|
|
|||
| @WebMvcTest | |||
| @WebMvcTest(WorkTimerController.class) | |||
There was a problem hiding this comment.
👍🏻👍🏻👍🏻 이렇게 처리를 하면, 해댕 컨트롤러와, 관련 bean 들을 등록해서 스부테보다는 가볍게 테스트 환경을 꾸릴 수 있더라구요.
| } | ||
|
|
||
| // 삭제 상태 확인 | ||
| private static void deleteStatus(todo todo) { |
There was a problem hiding this comment.
삭제 상태를 체크한다는 의미로 저는 해석이 되었습니다 👍🏻
| @Column(name = "ID", nullable = false) | ||
| private Long id; | ||
|
|
||
| @Column(name = "TODO", nullable = false) |
There was a problem hiding this comment.
훨씬 더 이해하기 좋은 코드가 된 것 같아요 👍🏻 감사합니다!
| this.todo = todo; | ||
| } | ||
| public void delete (boolean delete) { | ||
| this.delete = delete; |
There was a problem hiding this comment.
객체 상태(delete 여부)는 객체 자신(todo)이 바꿔야 된다고 생각을 하는 편입니다. 이래야 더 안전하고 좀 더 객체지향적인 코드를 작성을 한다고 저는 생각을 하고 있거든요 😂
저의 경우는 해당 코드를 아래와 같이 작성할 것 같아요
public void delete() {
this.delete = true;
}지금 주철님의 코드 경우에는 service계층에서 delete를 통해서 true를 넘겨주지만,
혹시 누가 몰래 false를 넣어주는 코드를 실수로 넣었다고 하면, 이를 또 하나씩 찾아야 하잖아요.
다행인건 해당 메서드는 boolean으로 넣어질 수 있는 값이 한정적이지만, String 같은걸로, 혹은 값이 다양한 enum 형식이였다면, 충분히 상태가 잘못될 수 있는 상황을 열어주는 것이라고 봅니다.
외부라는 건, 다른 클래스가 될 수도 있을 것 같네요.
혹시 질문에 답변이 되었을까요?
저장(post) : /todo
수정(patch) : /todo/{id}
삭제(delete) : /todo/{id}
다짐 수행 여부 체크(post) : /todo/{id}