-
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
[feat] #172 - promotion 필드 추가 및 지난 공연 캐러샐 자동 삭제 구현, response에 dueDate 추가 #175
Conversation
…response에 schedule별 dueDate 추가
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.
고생하셨습니다!
dueDate에 대해서 많은 고민을 하신 점이 인상 깊었습니다.
수정할 부분에 대해서 코멘트 남겨 두었으니 확인 부탁드려요~
return new PerformanceDetailResponse( | ||
performanceId, |
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.
return new PerformanceDetailResponse(performanceId, performanceTitle, performancePeriod, scheduleList, ticketPrice, genre, posterImage, runningTime, performanceVenue, performanceDescription, performanceAttentionNote, performanceContact, performanceTeamName, castList, staffList, minDueDate);
이런 식으로 공백 제거 말씀하시는 거 맞을까요?
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.
엇 제 코드 상으론 공백 없는 채로 커밋했는데 왜 이렇게 보일까요ㅠㅠ 다시 커밋해보겠습니다
for (Promotion promotion : promotions) { | ||
if (promotion.getPerformance() != null) { | ||
Performance performance = promotion.getPerformance(); | ||
|
||
List<Schedule> schedules = scheduleRepository.findByPerformanceId(performance.getId()); | ||
int minDueDate = scheduleService.getMinDueDate(schedules); | ||
|
||
// MinDueDate가 음수일 경우 Promotion 삭제 | ||
if (minDueDate < 0) { | ||
promotionRepository.delete(promotion); | ||
} |
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.
promotion.getPerformance() != null일 때 promotion.getPerformance()을 하는게 어색합니다.
우선순위가 promotion.getPerformance()을 한 후 null 체크를 하는게 맞을 것 같습니다!
코드를 아래와 같이 바꾸면 더 좋을 것 같다는 생각이 듭니다
@Scheduled(cron = "1 0 0 * * ?")
@Transactional
public void checkAndDeleteInvalidPromotions() {
List<Promotion> promotions = promotionRepository.findAll();
for (Promotion promotion : promotions) {
Performance performance = promotion.getPerformance();
if (performance != null) {
List<Schedule> schedules = scheduleRepository.findByPerformanceId(performance.getId());
int minDueDate = scheduleService.getMinDueDate(schedules);
// MinDueDate가 음수일 경우 Promotion 삭제
if (minDueDate < 0) {
promotionRepository.delete(promotion);
}
}
}
}
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.
외부링크와 연결된 promotion의 경우 연관된 performance가 없기 때문에 먼저 performance를 저장하지 않고 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.
로직상은 둘다 문제가 없지만, 코드를 작성할때는 가독성과 유지보수성을 고려하여 설계를 하여야 합니다.
for문 바로 아래 if (promotion.getPerformance() != null) 로 공연이 null이 아닐 때 로직 수행을 하게 되는데, 이는 확장성이 부족한 코드라고 생각됩니다.
향후 다른 조건에 따라 프로모션을 처리해야 할 경우, 조건문이 계속해서 복잡해지고, 메서드의 가독성이 떨어질 수 있습니다.
private void processPromotion(Promotion promotion) {
Performance performance = promotion.getPerformance();
if (performance == null) {
return; // performance가 null이면 메서드를 종료
}
List<Schedule> schedules = scheduleRepository.findByPerformanceId(performance.getId());
int minDueDate = scheduleService.getMinDueDate(schedules);
if (minDueDate < 0) {
promotionRepository.delete(promotion);
}
}
저는 위와 같은 방식으로 공연을 가져오고, null이면 얼리 리턴을 하는 방식을 추천드립니다.
향 후 null일 때, 처리로직이 변경되도 확장성을 고려해서 설계하였기 때문에 if문 안에 조건 블록 내에서 로직을 추가/변경하면 되겠습니다.
또한, 불필요한 중복을 줄일 수 있고, performance가 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.
확실히 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.
필드를 추가하면 테이블 컬럼 내용이 변경이 되야 하니, 직접 Dev, Prod DB에 칼럼을 추가해야 하는점 알고 계시면 좋을 것 같습니다!
제가 추가하는 법 알려드릴게요~
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.
감사합니다ㅎㅎ
@@ -20,19 +20,29 @@ public class Promotion { | |||
private String promotionPhoto; | |||
|
|||
@ManyToOne(fetch = FetchType.LAZY) | |||
@JoinColumn(name = "performance_id", nullable = false) | |||
@JoinColumn(name = "performance_id", nullable = 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.
이 부분은 공연 id 대신 외부 링크 url이 사용될 수 있어서 null=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.
네 맞습니다!
return new PerformanceDetailResponse( | ||
performanceId, |
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.
for (Promotion promotion : promotions) { | ||
if (promotion.getPerformance() != null) { | ||
Performance performance = promotion.getPerformance(); | ||
|
||
List<Schedule> schedules = scheduleRepository.findByPerformanceId(performance.getId()); | ||
int minDueDate = scheduleService.getMinDueDate(schedules); | ||
|
||
// MinDueDate가 음수일 경우 Promotion 삭제 | ||
if (minDueDate < 0) { | ||
promotionRepository.delete(promotion); | ||
} |
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.
로직상은 둘다 문제가 없지만, 코드를 작성할때는 가독성과 유지보수성을 고려하여 설계를 하여야 합니다.
for문 바로 아래 if (promotion.getPerformance() != null) 로 공연이 null이 아닐 때 로직 수행을 하게 되는데, 이는 확장성이 부족한 코드라고 생각됩니다.
향후 다른 조건에 따라 프로모션을 처리해야 할 경우, 조건문이 계속해서 복잡해지고, 메서드의 가독성이 떨어질 수 있습니다.
private void processPromotion(Promotion promotion) {
Performance performance = promotion.getPerformance();
if (performance == null) {
return; // performance가 null이면 메서드를 종료
}
List<Schedule> schedules = scheduleRepository.findByPerformanceId(performance.getId());
int minDueDate = scheduleService.getMinDueDate(schedules);
if (minDueDate < 0) {
promotionRepository.delete(promotion);
}
}
저는 위와 같은 방식으로 공연을 가져오고, null이면 얼리 리턴을 하는 방식을 추천드립니다.
향 후 null일 때, 처리로직이 변경되도 확장성을 고려해서 설계하였기 때문에 if문 안에 조건 블록 내에서 로직을 추가/변경하면 되겠습니다.
또한, 불필요한 중복을 줄일 수 있고, performance가 null이 아닌 경우에만 나머지 로직을 수행하기 때문에, 코드의 가독성이 더 좋아졌습니다.
… 및 확장성 고려한 refactor
Related issue 🛠
Work Description ✏️
Trouble Shooting ⚽️
Related ScreenShot 📷
Uncompleted Tasks 😅
To Reviewers 📢