feat: YSL-14 공지사항 생성 기능 추가 #15
Conversation
- EnableJpaAuditing Cofiguration으로 이동 - 생성과 관련 없는 조회부분 삭제 - 오타 수정
| @Configuration | ||
| @EnableJpaAuditing | ||
| public class JpaConfig { | ||
| } |
There was a problem hiding this comment.
마지막 라인이 없는것을 확인하고 올려주시면 좋을 것 같아요.
저는 기본적으로 개발을 다 하고나서 습관적으로 mac기준
[ Alt + Control + o ]로 사용하지 않는 import들 삭제해주고,
[ Alt + Command + l ]로 포맷팅 자동정렬 기능을 사용해주고 있습니다.
한 번 확인 해보세요
There was a problem hiding this comment.
다 같이 어떤 코드 스타일 사용할건지도 정하면 좋을 것 같네요 😀
| private final NoticeRepository noticeRepository; | ||
|
|
||
| @Transactional | ||
| public Long registerNotice(Notice notice){ |
There was a problem hiding this comment.
별거 아니지만, 메소드를 열어주는 중괄호의 경우, 파라미터 리스트의 닫는 소괄호 ")" 뒤와 여는 중괄호 "{" 사이에 하나의 공백이 있는 것이 메소드를 의미하는 컨벤션 중 하나라 이런 사소한 것도 같이 지켜나갔으면 좋겠어요.
관련글: 8.3
https://naver.github.io/hackday-conventions-java/#space-around-brace
There was a problem hiding this comment.
오 공백을 어떻게 해야하는지 궁금했는데 컨벤션이 있었네요 :)
참고해서 수정하겠습니다
| @Entity | ||
| @Getter | ||
| public class Notice extends AuditInformation { | ||
| @Id @GeneratedValue(strategy=GenerationType.IDENTITY) |
There was a problem hiding this comment.
어노테이션은, 짧더라도 두줄에 걸쳐 사용하는게 더 직관적이라 좋을 것 같습니다.
| private String content; | ||
|
|
||
| /* | ||
| @ManyToOne(fetch=FetchType.LAZY) |
There was a problem hiding this comment.
언제 사용될지 모르는 내용에 대해서는 주석이 아닌, 삭제를
혹은 빠르게 바뀔 내용에 대해서는 TODO로 작성을 하기를 권합니다.
| public class Notice extends AuditInformation { | ||
| @Id @GeneratedValue(strategy=GenerationType.IDENTITY) | ||
| private Long id; | ||
| @Column(length=50, nullable = false) |
There was a problem hiding this comment.
자동 줄바꿈 단축키를 생활화하면 이런 문제는 다신 안만나겠지만,
당연 줄바꿈도 잘 해주시면 좋을 것 같아요
|
|
||
| @PostMapping("/notice") | ||
| public ResponseEntity<Void> registerNotice(@RequestBody NoticeSaveRequest request){ | ||
| Notice notice = Notice.createNotice(request.getTitle(), request.getContent()); |
There was a problem hiding this comment.
나중에 Notice의 내용이 더 추가가 된다면 매번 get으로 꺼내어 변경이 될 텐데
이왕 NoticeSaveRequest라는 DTO로 받았으니, DTO 그대로 넘겨주는건 어떨까요?
다른 분들의 의견도 궁금하네요.
There was a problem hiding this comment.
변경하고 나서 코멘트를 확인했네요
저도 뭔가 이상해서 DTO로 전달하는 것으로 변경했습니다!
| Notice notice = Notice.createNotice(request.getTitle(), request.getContent()); | ||
| Long response = noticeService.registerNotice(notice); | ||
|
|
||
| // body: ? |
There was a problem hiding this comment.
만약 PR 단계에서 묻고 싶은 이야기가 있다면 주석으로 언제든 남겨주세요.
사실 이 내용은 어떤걸 묻고자하는지 잘 모르겠네요 😂
There was a problem hiding this comment.
body에 다른 것이 더 안들어가도 되는지 궁금해서 주석을 남겼던 것 같습니다.
생성시에 조회할 수 있는 URI만 body에 포함시키면 될까요?
| NoticeRepository noticeRepository; | ||
|
|
||
| @Nested | ||
| @DisplayName(value="공지 저장") |
There was a problem hiding this comment.
value는 생략이 가능하고,
이름 짓는거가 생각보다 서술식으로 쓰는게 처음은 어색하더라구요.
@nested를 쓰게 되면, 서술식으로 공지사항을 저장할 때~ 라던가 좀 더 읽혀지는 설명을 써보시는걸 추천드려봅니다.
나중에 다시 Notice 부분을 개선하더라도 금방 다시 파악하는데에 큰 도움이 될 거에요
There was a problem hiding this comment.
변경했습니다.
value는 생략하는 것이 좋을까요? 아니면 명시적으로 나타내는 것이 좋을까요?
There was a problem hiding this comment.
저는 생략하고 쓰는 것 같습니다
저희는 작은 프로젝트라 크게 상관은 없겠지만, 회사에서는 팀의 컨벤션에 맞추어 작성하면 좋을 것 같아요 👍🏻
| @DisplayName("정상적으로 저장") | ||
| void saveNotice() { | ||
| // given | ||
| Notice notice = Notice.createNotice("test title", "test content"); |
There was a problem hiding this comment.
title과 content를 동일하게 밑에 then절에서 검증을 하고 있는데, 이런건 given절에서 공통 변수로 빼보는건 어떨까요?
//given
final String title = "test title";
final String content = "test content";
final Notice notice = Notice.createNotice(title, content);
...
| Notice notice = Notice.createNotice(null, "test content"); | ||
| //when | ||
| //then | ||
| Assertions.assertThatThrownBy(() -> noticeRepository.save(notice)).isInstanceOf(RuntimeException.class); |
There was a problem hiding this comment.
when과 then이 동시에 있다면 저는 가독성을 위해, //when & then으로 자주 사용합니다 😀
There was a problem hiding this comment.
어떤 테스트 코드 관련 강의에서는 when, then 있으면 그냥 expected 로 두개 떼우기도 하더라구요.
어차피 검증부라서 저도 expected 라는게 어울리는것 같아서 given / when / then 패턴이서는 그렇게 사용하고 있습니다.
참고하시면 좋을것 같아요 .
| NoticeRepository noticeRepository; | ||
|
|
||
| @Nested | ||
| @DisplayName(value="공지 저장") |
There was a problem hiding this comment.
저는 생략하고 쓰는 것 같습니다
저희는 작은 프로젝트라 크게 상관은 없겠지만, 회사에서는 팀의 컨벤션에 맞추어 작성하면 좋을 것 같아요 👍🏻
| @Entity | ||
| @Getter | ||
| public class Notice extends AuditInformation { | ||
| @Id @GeneratedValue(strategy=GenerationType.IDENTITY) |
| Assertions.assertThat(findNotice).isNotNull(); | ||
| Assertions.assertThat(findNotice.getTitle()).isEqualTo("test title"); | ||
| Assertions.assertThat(findNotice.getContent()).isEqualTo("test content"); |
There was a problem hiding this comment.
Assertions과 SoftAssertions의 차이를 혹시 아시나요?
Assertions을 통해서 검사를 할 때, 두번째 줄의 검증에서 fail이 떨어진다면, 뒤의 테스트에 대해서는 어떻게 되는지 궁금하네요.
There was a problem hiding this comment.
Assertions은 중간에 fail이 떨어지면 중지되고, SoftAssertions는 일단 전부 다 실행해보네요:)
감사합니다😀
There was a problem hiding this comment.
오 맞아요. 그리고 softly를 쓰면, 어디 부분에서 오류가 났는지도 알려주더라구요! 😄 한 번 비교해가면서 써보심을 추천드립니다👍🏻
공지사항 생성하는 기능입니다
수정 및 추가해야할 것이 필요하면 말씀해주세요 :)