MOSU-19 feat: 신청 기능 구현 #41
Conversation
polyglot-k
left a comment
polyglot-k
left a comment
There was a problem hiding this comment.
전체적으로 깔끔하게 잘 짜신 것 같습니다!
일단 Merge 진행 이후에, 이 부분 처리한 걸로 PR 한 번 더 올리는 식으로 작업해주시면 감사하겠습니다!
아래의 부분을 조금 더 고려해보세요
😎 long(primitive) 과 Long(Wrapper Class) 는 언제 써야할까? (boolean 도 마찬가지)
-> Auto-Boxing 과 Auto-UnBoxing 에 대한 개념과 이에 따른 오버헤드를 찾아보세요
😎 valid 가 필요한 형태는 뭐가 있을까?
-> Presentation Layer 에서 값의 형식을 보장해야하는 부분에서 대해서 고려해보세요
고생하셨습니다
| @Transactional | ||
| public ApplicationResponse apply(Long userId, ApplicationRequest request) { | ||
| Set<ApplicationSchoolRequest> schools = request.schools(); | ||
| List<ApplicationSchoolJpaEntity> schoolEntities = new java.util.ArrayList<>(List.of()); |
There was a problem hiding this comment.
java.util 은 import 로 처리하면 될거 같아요~ :)
ArrayList<> 로 바로 써보는건 어떨까요?그리고, List.of() 를 안에 굳이 안넣어도 빈 배열로 들어갑니다!
|
|
||
| schools.forEach(applicationSchoolRequest -> { | ||
| ApplicationSchoolJpaEntity applicationSchoolJpaEntity = applicationSchoolRequest.toEntity(userId, applicationId); | ||
| schoolEntities.add(applicationSchoolJpaRepository.save(applicationSchoolJpaEntity)); |
There was a problem hiding this comment.
요거는 간단한 컨벤션인데, add나 save같은거 한줄로 적을 때 분리해서 적으면 가독성이 더 좋을 것 같아요!
|
|
||
| return applications.stream() | ||
| .map(application -> { | ||
| List<ApplicationSchoolJpaEntity> schools = applicationSchoolJpaRepository.findAllByApplicationId(application.getId()); |
There was a problem hiding this comment.
application.getId() 를 Long 으로 빼도 좋고, 그냥 해도 괜찮을거 같아요 :)
이거는 편하게 해주세요
|
|
||
| @Transactional | ||
| public ApplicationSchoolResponse updateSubjects( | ||
| @RequestParam Long userId, |
There was a problem hiding this comment.
RequestParam은 필요없어보이네요! Controller에서 했으면 여기서는 따로 적지 않아도 됩니더
|
|
||
| @Transactional | ||
| public void cancelApplicationSchool( | ||
| @RequestParam Long userId, |
| public ApiResponseWrapper<List<ApplicationResponse>> getAll( | ||
| @RequestParam Long userId | ||
| ) { | ||
| List<ApplicationResponse> responses = applicationService.getApplications(userId); |
There was a problem hiding this comment.
동일 기능이면, Controller와 Service가 이름이 비슷하면 조금 더 좋을거 같아요!
비즈니스 레이어와 컨트롤러의 시각 차이라서 메소드이름을 꼭 다르게 생각하지 않아도 좋을거 같아요
| public record AddressRequest( | ||
| String zipcode, | ||
| String street, | ||
| String detail | ||
| ) { | ||
| public AddressJpaVO toValueObject() { | ||
| return AddressJpaVO.builder() | ||
| .zipcode(zipcode) | ||
| .street(street) | ||
| .detail(detail) | ||
| .build(); | ||
| } | ||
| } No newline at end of file |
|
|
||
| public record ApplicationRequest( | ||
| FileRequest admissionTicket, | ||
| String recommenderPhoneNumber, |
There was a problem hiding this comment.
이부분 Pattern 으로 valid 처리해주면 안정성이 좋을거 같아요
| public record ApplicationSchoolRequest( | ||
| Long schoolId, | ||
| String schoolName, | ||
| String area, | ||
| AddressRequest address, | ||
| LocalDate examDate, | ||
| String lunch, | ||
| Set<String> subjects | ||
| ) { |
There was a problem hiding this comment.
이 부분도 valid 처리하면 안정성이 좋을거 같아요!
| private Set<Subject> validatedSubjects(Set<String> subjects) { | ||
| try { | ||
| return subjects.stream() | ||
| .map(Subject::valueOf) | ||
| .collect(Collectors.toSet()); | ||
| } catch (IllegalArgumentException e) { | ||
| throw new CustomRuntimeException(ErrorCode.WRONG_SUBJECT_TYPE); | ||
| } | ||
| } | ||
|
|
||
| private Lunch validatedLunch(String lunch) { | ||
| try { | ||
| return Lunch.valueOf(lunch.toUpperCase()); | ||
| } catch (IllegalArgumentException e) { | ||
| throw new CustomRuntimeException(ErrorCode.WRONG_LUNCH_TYPE); | ||
| } | ||
| } | ||
|
|
||
| private Area validatedArea(String area) { | ||
| try { | ||
| return Area.valueOf(area.toUpperCase()); | ||
| } catch (IllegalArgumentException e) { | ||
| throw new CustomRuntimeException(ErrorCode.WRONG_AREA_TYPE); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Enum Valid 처리 너무 잘했어요 :) ✅✅
✨ 구현한 기능
📢 논의하고 싶은 내용
🎸 기타