-
Notifications
You must be signed in to change notification settings - Fork 0
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
#13 Feature: 일정 관련 로직 구현 #14
Conversation
@Transactional(readOnly = true) | ||
public ResponseEvent getEventById(Long id) { | ||
Event event = eventRepository.findById(id) | ||
.orElseThrow(() -> new EntityNotFoundException("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.
에러 메시지를 enum으로 관리하거나 커스텀 exception 안에 Message를 고정해두는 방식이 관리하기 더 좋을것 같습니다.
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.
제가 생각하기엔 커스텀 exception 안에 메시지를 고정하면 다른 도메인에서 사용시 에러 메시지가 명시적으로 전달되기 어려울 거 같아서 도메인 별로 에러 메시지를 enum으로 관리하는 것이 나을 거 같습니다! 해당 방식으로 수정하겠습니다! 다른 방법이 있다면 알려주시면 감사하겠습니다!!
List<Event> events = eventRepository.findByStartDateTimeBetween(startDate, endDate); | ||
return events.stream() | ||
.map(eventMapper::toDto) | ||
.collect(Collectors.toList()); |
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.
데이터의 불변성을 지키기 위해 toList()
를 사용해보시면 좋을 것 같아요 관련 링크 남겨둘게요
https://www.inflearn.com/questions/986386/%EC%8A%A4%ED%8A%B8%EB%A6%BC-tolist%EC%99%80-collect%EC%9D%98-%EC%B0%A8%EC%9D%B4
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 | ||
public void updateEvent(Long id, RequestEvent requestEvent) { | ||
Event oldEvent = eventRepository.findById(id) | ||
.orElseThrow(() -> new EntityNotFoundException("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.
위 코멘트와 마찬가지로 enum으로 관리하면 더 좋을거 같아요
// 일정 삭제 | ||
public void deleteEvent(Long id) { | ||
if (!eventRepository.existsById(id)) { | ||
throw new EntityNotFoundException("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.
여기도요!
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.
고생하셨습니다!
근표님이 좋은 리뷰를 많이 달아주셔서, 제가 할 말이 딱히 별로 없네요 ㅎㅎ
import java.time.LocalDateTime; | ||
|
||
@Getter | ||
@Setter |
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.
record를 통해 DTO 클래스를 구현해준다면 더욱 좋을 것 같습니다!
DTO 클래스가 가져야 하는 모든 책임들을 포함하고 있습니다 :)
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.
이미 여러개의 리뷰가 달려서 간소하게 달았습니다!
- 당연히 테스트 후 PR을 올리시겠지만, 제가 생각하기에 스크린샷도 함께 첨부한다면 서로 어떤 데이터를 주고 받는지 다른 팀원들도 확인하시기 편한것 같아요(이건 제 개인적인 의견입니다!)
2.인증인가가 없는데, 유저에 대한 부분이 없어도 되는지? 궁금합니다.
3.swagger를 이미 도입하신걸로 아는데 전체적으로 그부분에 대해서 활용도가 적으신거 같아요ㅠ! 문서화하신 만큼 예를 들어 id가 일정에 대한 id이고, 이 API는 어떤 API 이고, 에러에 대한 설명도 같이 있으면 좋을 것 같아요:)
4.제가 느끼기에 일정 조회같은 경우에는 더 많은 데이터가 필요할것 같지만... ! 제가 기능명세서나 피그마 한번 더 확인 후 남기도록 하겠습니다!
고생하셨습니다!
return CommonResponse.createSuccess(responseEvents); | ||
} | ||
|
||
// 일정 수정 |
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.
관리자만 수정할 수 있게 구현할 예정입니다! 아직 인증인가를 구현하지 않았습니다!
@RequestParam(name = "start") @DateTimeFormat(iso = DateTimeFormat.ISO.DATE_TIME) LocalDateTime startDate, | ||
@RequestParam(name = "end") @DateTimeFormat(iso = DateTimeFormat.ISO.DATE_TIME) LocalDateTime endDate) { |
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.
ISO.DATA_TIME 으로 맞춘 점이 좋네요! 프론트와 데이터 타입이 맞지 않는 경우가 많은데, 신경쓰신거 같아서 좋은것 같습니다:)
import java.time.LocalDateTime; | ||
|
||
@Getter | ||
@Setter |
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.
dto 에 setter는 아래의 이유로 쓰지 않는다고 하네요.
1)Setter 메소드를 사용하면 값을 변경한 의도를 파악하기 힘듭니다.
2)객체의 일관성을 유지하기 어렵습니다.
대신 이렇게 많이 대신 쓰이기도 한다고 합니다.
1)생성자를 오버로딩
2)Builder 패턴 사용
3)정적 팩토리 메소드
꼭 필요한 어노테이션인지 확인하시면 좋을것 같아요:)
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.
양 쪽 DTO 모두 사용하지 않아서 setter는 제거하였습니다!
oldEvent.update( | ||
requestEvent.getTitle(), | ||
requestEvent.getContent(), | ||
requestEvent.getLocation(), | ||
requestEvent.getStartDateTime(), | ||
requestEvent.getEndDateTime() | ||
); | ||
} |
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.
이부분은 왜 MapStruct을 사용하지 않는건지 알수 있을까요?
update 할 부분이 많아 지는 경우에는 모든부분을 고칠 필요가 없는 것 같아요, 유저에 따라서 일부분만 변경 하고 싶을 수도 있다고 생각합니다. 이부분에 대해서 어떻게 생각하시는지 궁금하구요!
그래서 저같으면, mapstruct을 이미 사용하고 계시기 때문에 request값이 null이 아닌 부분만 update를 할 수 있지 않을까 생각했습니다:)
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도 같이 업데이트 되고 있어서 더 연구해보고 수정하도록 하겠습니다!
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.
찾아보니 MapStruct를 이용해 update 하는 방식은 Event Entity에 Setter를 모두 열어놔야하는 방식이라서 다른 방식으로 구현해봤는데 한 번 확인해주실 수 있으실까요??
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 EventService eventService; | ||
|
||
@Operation(summary = "일정 생성", description = "관리자가 일정을 등록합니다.") | ||
@PreAuthorize("hasRole('ROLE_USER') or hasRole('ROLE_ADMIN')") |
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.
@PreAuthorize("hasAnyAuthority('ROLE_USER', 'ROLE_ADMIN')")
로 둘 중에 하나만 일치하면 인가 되도록 할 수 있을거에요
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.
개발 편의 상 둘 다 열어두는 쪽이 편할 거 같아서 열어뒀습니다!
|
||
@Operation(summary = "일정 삭제", description = "관리자가 일정을 삭제합니다.") | ||
@PreAuthorize("hasRole('ROLE_USER') or hasRole('ROLE_ADMIN')") | ||
@DeleteMapping("/{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.
create와 update 요청과는 다르게 왜 delete는 주소에 delete가 없는건가요??
POST 요청은 다양한 목적을 가진 요청일 가능성이 크므로 주소에 create가 들어가는 것도 괜찮을 것 같다고 생각해요
하지만 update와 delete는 HTTP 메서드로 충분히 목적을 밝히고 있는 것 같아 굳이 넣을 필요가 없다고 생각했어요
권장하는 RESTful API 설계 중 "행위(method)는 URL에 포함하지 않는다."는 방법을 따르는 건 어떨까요?
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.
참고해서 수정해보겠습니다
|
||
List<Event> findByStartDateTimeBetween(LocalDateTime startDate, LocalDateTime endDate); | ||
|
||
Optional<Event> findById(Long 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.
SimpleJpaRepository
에 이미 구현된 메서드를 다시 구현한 이유가 있나요??
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) | ||
public ResponseEvent getEventById(Long id) { | ||
Event event = eventRepository.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.
레포지토리에 달았던 코멘트와 같이 SimpleJpaRepository
에 구현된 메서드를 가져다 쓰면 이미 @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.
이것도 몰랐습니다..
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)는 명시적으로 표시하는 게 좋을 거 같아서 성능상 차이가 없다며 남겨두겠습니다!
1. 개발내용
✅ POST: 일정 생성
✅ GET: 일정 상세 조회
✅ GET: 기간 별 일정 조회
✅ PATCH: 일정 수정
✅ DELETE: 일정 삭제
2. 구현 세부사항
3. 고민