- 
                Notifications
    
You must be signed in to change notification settings  - Fork 0
 
[Refactor] 기수 관리 추가 및 기존 기능과 연동 #96
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
Conversation
7a40405    to
    9f31849      
    Compare
  
    …게 구현 및 테스트 코드에 반영 (DASOMBE-16)
0079bfe    to
    45abfc2      
    Compare
  
    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.
작업 고생하셨습니다! 아래 사항 참고 부탁드립니다.
- 
GenerationService의 역할
GenerationService는 모집 기수 관련 로직을 포함하고 있는데, RecruitService가 이미 부원 모집 로직을 다루고 있으므로 RecruitService에 통합하는 것이 더 자연스러워 보입니다. - 
패키지 구조(global → domain)
global 패키지는 특정 도메인에 종속되지 않는 공용 처리만을 위한 공간입니다. 따라서 global/generation 보다는 domain/generation으로 이동하는 것이 패키지 구조에 더 적합합니다. - 
Generation 엔티티 관리 방식
Generation이 별도의 엔티티로 정의되어 있음에도 모집 기수를 RecruitRepository에서 조회하는 부분(RecruitServiceImpl 65번째 행)은 의도가 불분명해 보입니다. 개인적으로는 Recruit에서 모집 설정과 함께 기수까지 한 번에 관리하는 방식이 더 일관성 있어 보입니다. 
리뷰 요구사항 피드백
| if(config.getKey() == ConfigKey.GENERATION) { //기수 조회 추가 | ||
| String currentGeneration = generationService.getCurrentGeneration(); | ||
| return RecruitConfigResponseDto.builder() | ||
| .key(ConfigKey.GENERATION) | ||
| .value(currentGeneration) | ||
| .build(); | ||
| } else if(config.getKey() == ConfigKey.INTERVIEW_TIME_START || config.getKey() == ConfigKey.INTERVIEW_TIME_END) { | ||
| return config.toTimeResponse(); | ||
| } else { | ||
| return config.toResponse(); | ||
| } | ||
| }) | 
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.
if-else 문이 길어질 경우 switch-case 문을 활용해보세요
| 
               | 
          ||
| } | ||
| // 기수 정보를 저장할 필드 추가 | ||
| @Column(name = "generation", length = 16, nullable = false) | 
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.
기수 번호는 2자리수인데 속성 길이가 16이나 필요할까요?
…기수를 입력받고 값이 없을경우, 현재 모집일정의 기수를 기입하게 수정 (DASOMBE-16)
| 
           기수 기능 Recruit에 통합 및 회원가입시 선택적으로 기수 입력기능 추가, 코드 정리 완료했습니다.  | 
    
Issue
변경 내용
테스트
기수 수정 정상작동 확인
회원가입 정상 작동 및 기수 기능 작동 확인
리뷰 요구사항(필요시)