-
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
Changes from 8 commits
80136b3
ca3c4b4
a952d27
ee2cb4e
1f4fe87
971992e
ac5a6a7
7d88deb
94bf32b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,43 @@ | ||
package com.beat.domain.promotion.application; | ||
|
||
import com.beat.domain.performance.domain.Performance; | ||
import com.beat.domain.promotion.dao.PromotionRepository; | ||
import com.beat.domain.promotion.domain.Promotion; | ||
import com.beat.domain.schedule.application.ScheduleService; | ||
import com.beat.domain.schedule.dao.ScheduleRepository; | ||
import com.beat.domain.schedule.domain.Schedule; | ||
import lombok.RequiredArgsConstructor; | ||
import org.springframework.scheduling.annotation.Scheduled; | ||
import org.springframework.stereotype.Service; | ||
import org.springframework.transaction.annotation.Transactional; | ||
|
||
import java.util.List; | ||
|
||
@Service | ||
@RequiredArgsConstructor | ||
public class PromotionService { | ||
|
||
private final PromotionRepository promotionRepository; | ||
private final ScheduleRepository scheduleRepository; | ||
private final ScheduleService scheduleService; | ||
|
||
@Scheduled(cron = "1 0 0 * * ?") | ||
@Transactional | ||
public void checkAndDeleteInvalidPromotions() { | ||
List<Promotion> promotions = promotionRepository.findAll(); | ||
|
||
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 commentThe 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 commentThe 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 commentThe 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이면 얼리 리턴을 하는 방식을 추천드립니다. 또한, 불필요한 중복을 줄일 수 있고, performance가 null이 아닌 경우에만 나머지 로직을 수행하기 때문에, 코드의 가독성이 더 좋아졌습니다. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more. 감사합니다ㅎㅎ |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more. 네 맞습니다! |
||
private Performance performance; | ||
|
||
@Column(nullable = true) | ||
private String redirectUrl; | ||
|
||
@Column(nullable = false) | ||
private boolean isExternal; | ||
|
||
@Builder | ||
public Promotion(String promotionPhoto, Performance performance) { | ||
public Promotion(String promotionPhoto, Performance performance, String redirectUrl, boolean isExternal) { | ||
this.promotionPhoto = promotionPhoto; | ||
this.performance = performance; | ||
this.redirectUrl = redirectUrl; | ||
this.isExternal = isExternal; | ||
} | ||
|
||
public static Promotion create(String promotionPhoto, Performance performance) { | ||
public static Promotion create(String promotionPhoto, Performance performance, String redirectUrl, boolean isExternal) { | ||
return Promotion.builder() | ||
.promotionPhoto(promotionPhoto) | ||
.performance(performance) | ||
.redirectUrl(redirectUrl) | ||
.isExternal(isExternal) | ||
.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.
해당 공백 제거 부탁드릴게요!
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.
아뇨 ㅎㅎㅎ
이런식으로 줄바꿈이 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.
엇 제 코드 상으론 공백 없는 채로 커밋했는데 왜 이렇게 보일까요ㅠㅠ 다시 커밋해보겠습니다