Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,8 @@ public CreateApplicationResponse apply(Long userId, ApplicationRequest request)

Set<Subject> subjects = request.getSubjects();

validator.ExamDateNotPassed(examIds);
validator.examNotFull(examIds);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

It's good to see the addition of validator.examNotFull(examIds) to prevent applications for full exams. This enhances the application process by preventing users from applying to exams that have reached their capacity.

validator.examDateNotPassed(examIds);
validator.requestNoDuplicateExams(examIds);
validator.examIdsAndLunchSelection(request.examApplication());
validator.noDuplicateApplication(userId, examIds);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,10 @@
import java.time.LocalDate;
import java.util.HashSet;
import java.util.List;
import java.util.Optional;
import java.util.Set;
import java.util.stream.Collectors;
import life.mosu.mosuserver.application.exam.cache.ExamQuotaCacheManager;
import life.mosu.mosuserver.domain.application.ApplicationJpaRepository;
import life.mosu.mosuserver.domain.exam.ExamJpaEntity;
import life.mosu.mosuserver.domain.exam.ExamJpaRepository;
Expand All @@ -20,6 +22,7 @@ public class ApplicationValidator {

private final ExamJpaRepository examJpaRepository;
private final ApplicationJpaRepository applicationJpaRepository;
private final ExamQuotaCacheManager examQuotaCacheManager;

public void requestNoDuplicateExams(List<Long> examIds) {
Set<Long> examIdSet = new HashSet<>(examIds);
Expand All @@ -30,7 +33,7 @@ public void requestNoDuplicateExams(List<Long> examIds) {

public void examIdsAndLunchSelection(List<ExamApplicationRequest> requests) {
if (requests == null || requests.isEmpty()) {
throw new CustomRuntimeException(ErrorCode.EXAM_APPLICATION_NOT_FOUND);
throw new CustomRuntimeException(ErrorCode.EXAM_NOT_FOUND);
}

List<Long> requestedExamIds = requests.stream()
Expand Down Expand Up @@ -68,7 +71,7 @@ public void noDuplicateApplication(Long userId, List<Long> examIds) {
}
}

public void ExamDateNotPassed(List<Long> examIds) {
public void examDateNotPassed(List<Long> examIds) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Renaming ExamDateNotPassed to examDateNotPassed improves code readability by adhering to Java naming conventions for methods (camelCase).

List<ExamJpaEntity> exams = examJpaRepository.findAllById(examIds);
boolean hasPassedExam = exams.stream()
.anyMatch(exam -> exam.getExamDate().isBefore(LocalDate.now()));
Expand All @@ -77,4 +80,22 @@ public void ExamDateNotPassed(List<Long> examIds) {
throw new CustomRuntimeException(ErrorCode.EXAM_DATE_PASSED);
}
}

public void examNotFull(List<Long> examIds) {
boolean isFull = examIds.stream()
.anyMatch(examId -> {
Optional<Long> currentApplications = examQuotaCacheManager.getCurrentApplications(
examId);
Optional<Long> maxCapacity = examQuotaCacheManager.getMaxCapacity(examId);

if (currentApplications.isPresent() && maxCapacity.isPresent()) {
return currentApplications.get() >= maxCapacity.get();
}
return false;
});
Comment on lines +86 to +95

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The lambda expression within anyMatch can be simplified to be more concise and idiomatic by using Optional.flatMap and Optional.map. This avoids explicit isPresent() and .get() calls, which improves readability.

        boolean isFull = examIds.stream()
                .anyMatch(examId -> examQuotaCacheManager.getMaxCapacity(examId)
                        .flatMap(maxCapacity -> examQuotaCacheManager.getCurrentApplications(examId)
                                .map(currentApplications -> currentApplications >= maxCapacity))
                        .orElse(false));


if (isFull) {
throw new CustomRuntimeException(ErrorCode.APPLICATION_CLOSED);
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,6 @@
import life.mosu.mosuserver.domain.examapplication.projection.ExamApplicationInfoProjection;
import life.mosu.mosuserver.domain.examapplication.projection.ExamTicketInfoProjection;
import life.mosu.mosuserver.domain.examapplication.service.ExamNumberGenerationService;
import life.mosu.mosuserver.domain.payment.PaymentJpaRepository;
import life.mosu.mosuserver.global.exception.CustomRuntimeException;
import life.mosu.mosuserver.global.exception.ErrorCode;
import life.mosu.mosuserver.infra.persistence.s3.S3Service;
Expand All @@ -39,7 +38,6 @@ public class ExamApplicationService {
private final ApplicationJpaRepository applicationJpaRepository;
private final ExamSubjectJpaRepository examSubjectJpaRepository;
private final ExamNumberGenerationService examNumberGenerationService;
private final PaymentJpaRepository paymentJpaRepository;
private final S3Service s3Service;
private final FixedQuantityDiscountCalculator calculator;

Expand Down Expand Up @@ -122,7 +120,7 @@ public ExamApplicationInfoResponse getApplication(Long userId, Long examApplicat
// examApplicationId);
List<ExamApplicationJpaEntity> examApplicationEntities = examApplicationJpaRepository.findByApplicationId(
applicationId);
Integer lunchCount = (int) examApplicationEntities.stream()
int lunchCount = (int) examApplicationEntities.stream()

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Changing the type of lunchCount from Integer to int is a good practice as it avoids potential NullPointerException and is more memory-efficient for primitive types.

.filter(ExamApplicationJpaEntity::getIsLunchChecked)
.count();

Expand Down Expand Up @@ -173,8 +171,11 @@ private void validateUser(Long userId, Long examApplicationId) {
}

private int getAppliedDiscountAmount(Integer totalAmount) {
log.info("total amount: {}", totalAmount);
return calculator.getAppliedDiscountAmount(totalAmount);
try {
return calculator.getAppliedDiscountAmount(totalAmount);
} catch (Exception ex) {
throw new CustomRuntimeException(ErrorCode.PRICE_LOAD_FAILURE);
}
Comment on lines +174 to +178

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

Catching a generic Exception is too broad and can mask unexpected runtime exceptions. It's better to catch the specific exception(s) you expect, such as IllegalArgumentException which is thrown by FixedQuantityDiscountCalculator. This makes error handling more robust and predictable.

        } catch (IllegalArgumentException ex) {

}

private void validateExamTicketOpenDate(LocalDate examDate, String examNumber) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ public interface RefundJpaRepository extends JpaRepository<RefundJpaEntity, Long
JOIN ExamApplicationJpaEntity ea ON r.examApplicationId = ea.id
JOIN ExamJpaEntity e ON ea.examId = e.id
WHERE r.examApplicationId = :examApplicationId
AND p.paymentStatus = 'CANCELLED_DONE'
AND r.refundStatus = 'DONE'

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

The query condition p.paymentStatus = 'CANCELLED_DONE' seems to be replaced with r.refundStatus = 'DONE'. Ensure that this change accurately reflects the intended logic for filtering refunds. It's crucial that the correct status is used to identify refunds for the given exam application.

""")
Optional<RefundNotifyProjection> findRefundByExamApplicationId(
@Param("examApplicationId") Long examApplicationId);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,9 @@ public enum ErrorCode {
APPLICATION_SCHOOL_DUPLICATED(HttpStatus.BAD_REQUEST, "동일 일자의 같은 학교를 신청할 수 없습니다."),
EXAM_DUPLICATED(HttpStatus.BAD_REQUEST, "동일한 시험을 신청할 수 없습니다."),
WRONG_APPLICATION_ID_TYPE(HttpStatus.BAD_REQUEST, "신청 ID 타입이 올바르지 않습니다."),
PRICE_LOAD_FAILURE(HttpStatus.CONFLICT, "신청 가격 정보를 가져오는데 실패했습니다."),
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Reconsider HTTP status code for PRICE_LOAD_FAILURE.

Using HTTP 409 (Conflict) for price loading failure seems semantically incorrect. Consider using HTTP 500 (Internal Server Error) or HTTP 503 (Service Unavailable) instead, as this represents a system/service failure rather than a resource conflict.

-    PRICE_LOAD_FAILURE(HttpStatus.CONFLICT, "신청 가격 정보를 가져오는데 실패했습니다."),
+    PRICE_LOAD_FAILURE(HttpStatus.INTERNAL_SERVER_ERROR, "신청 가격 정보를 가져오는데 실패했습니다."),
🤖 Prompt for AI Agents
In src/main/java/life/mosu/mosuserver/global/exception/ErrorCode.java at line
67, the HTTP status code for PRICE_LOAD_FAILURE is currently set to 409
(Conflict), which is not appropriate for a system failure. Change the status
code to 500 (Internal Server Error) or 503 (Service Unavailable) to better
represent a service or system failure when price loading fails.

APPLICATION_CLOSED(HttpStatus.BAD_REQUEST, "신청이 마감되었습니다."),

// 프로필 관련 에러
PROFILE_ALREADY_EXISTS(HttpStatus.CONFLICT, "프로필이 이미 존재합니다."),
PROFILE_NOT_FOUND(HttpStatus.NOT_FOUND, "프로필을 찾을 수 없습니다."),
Expand Down Expand Up @@ -149,6 +152,8 @@ public enum ErrorCode {

//LUA 관련
LUA_SCRIPT_ERROR(HttpStatus.INTERNAL_SERVER_ERROR, "LUA 스크립트 실행 중 오류가 발생했습니다.");


private final HttpStatus status;
private final String message;
}