-
Notifications
You must be signed in to change notification settings - Fork 308
4단계 - 수강신청(요구사항 변경) #807
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
base: yangseungin
Are you sure you want to change the base?
4단계 - 수강신청(요구사항 변경) #807
Conversation
javajigi
left a comment
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.
마지막 단계에서 스트랭글러 패턴을 적용하기 위해 도전하는 모습이 인상적이네요. 💯
점진적인 리팩터링을 위한 도전의 흔적이 느껴져 넘 좋네요.
Q1.
- 스키마에 status와 progress_status, recruitment_status를 모두 넣어 스키마를 변경하고 상태값 세계의 컬럼의 중복을 발생시키면서 점진적으로 진행방법
- 기존의 SessionStatus에 진행중이면서 모집중, 진행중이면서 모집중이지않음 이런값들을 추가해서 스키마변경도 하지않고 간단하게 구현하는 방법
- 스키마의변경을 하지않고 도메인 객체의 분리만해서 만들어둔 ProgressStatus와 RecruitmentStatus로 분리해서 사용하는 방법
이 정도가 있을 것 같은데 처음 생각했던 방법은 스키마도 변경하고 객체에도 현재 PR시점에 상태인 status , ProgressStatus, Recruitmentstatus를 모두 존재하도록 진행하고 이후에 기존 DB데이터를 마이그레이션하여 새 컬럼으로 변환하고 그이후 Session에서도 Status를 제거하는 방식으로 진행하려 하는데 진행 방법에 대해 어떻게 생각하시나요?
A1.
이와 같은 변경이 발생할 경우 db 스키마를 변경하는 것에 두려움, 번거로움, 귀차니즘 등등 여러 요인으로 인해 2번과 3번 접근 방식으로 접근하는 경우가 많은 것 같아요.
그런데 이렇게 접근할 경우 추후에 분명 장애가 나거나 요구사항이 변경될 경우 복잡도가 계속해서 증가한다고 생각해요.
저도 승인님이 제안한 1번 방식으로 접근할 것을 추천합니다. 개발 순서는 다음과 같아 지겠죠?
- 스키마에 status와 progress_status, recruitment_status를 모두 추가. status 값을 progress_status, recruitment_status으로 마이그레이션
- 소스 코드를 ProgressStatus, Recruitmentstatus으로 리팩터링
- 코드 배포 후 문제가 없는지 확인.
- 문제 없으면 스키마에서 status 칼럼 삭제
|
|
||
| import java.util.Arrays; | ||
|
|
||
| public enum ProgressStatus { |
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.
status 분리 👍
|
|
||
| import java.util.Arrays; | ||
|
|
||
| public enum RecruitmentStatus { |
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.
status 분리 👍
| import java.util.List; | ||
| import java.util.Set; | ||
|
|
||
| public class Session { |
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.
Session 필드 수가 많음
그렇다보니 생성자의 인자수도 많음.
Session의 필드 수를 줄이거나 현재 상태를 유지할 수 밖에 없다면 생성자는 정말 필요한 수만 유지하면 어떨까?
생성자의 인자수가 많다보니 변경이 발생할 경우 영향을 받는 부분이 너무 많아지는 느낌이 든다.
|
|
||
| public Enrollment createEnrollment(List<EnrolledStudent> currentStudents) { | ||
| return new Enrollment(id, enrollment.getStatus(), enrollment.getSessionType(), currentStudents); | ||
| if (recruitmentStatus != 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.
recruitmentStatus 값이 null이 되도록 구현하는 것보다 기본 값을 NOT_RECRUITING와 같은 특정 상태를 가지도록 구현하는 것은 어떨까?
| Session session1 = new Session(1, new SessionPeriod(startDate, endDate), image, new Enrollment(SessionStatus.RECRUITING, new FreeSessionType())); | ||
| Session session2 = new Session(2, new SessionPeriod(startDate, endDate), image, new Enrollment(SessionStatus.RECRUITING, new FreeSessionType())); | ||
| Session session1 = new Session(startDate, endDate, image, "준비중"); | ||
| Session session2 = new Session(startDate, endDate, image, "준비중"); |
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.
Session을 생성하기 힘드니까 완전히 테스트를 위한 생성자를 추가한 느낌이 든다.
Session 객체와 같이 인스턴스 변수가 많은 객체를 테스트하려면 객체를 생성하는데 어려움이 있다.
중복 코드 또한 많이 발생해 Session을 생성할 때 생성자의 인자가 변경되는 경우 변경할 부분이 많아진다.
https://www.arhohuttunen.com/test-data-builders/ 문서 참고해 Session과 같이 복잡한 객체의 테스트 데이터를 생성할 때 어떤 방법을 사용할 것인지 선택해 보면 좋겠다.
이번 기회에 내가 선호하는 방법을 적용해 보고 앞으로도 쭈욱 활용하는 방식이면 좋겠다.
|
안녕하세요 주말간 가족여행이 있었어서 PR이 없었습니다. |
|
강사가 승인하지 않아도 수강 신청하는 모든 사람이 수강 가능하다 를 진행할때 수강신청의 사이클을 이런식으로 만들어봤습니다. 수강요청 -> 모집여부, 결제, 인원체크 후 수강신청서 생성 -> 상태체크 후 승인, 또는 취소 의 동작 지난주 강의에서 나왔던 패키지정리도 슬슬 해야할 시기가 온 것 같아 보여서 리뷰요청드리고 피드백 수정하면서 진행하겠습니다. |
javajigi
left a comment
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.
피드백 반영 하느라 수고했어요. 👍
추가로 피드백하고 싶은 부분이 있어 피드백 남겼어요.
pr 본문에 남긴 것처럼 패키지 분리 리팩터링 도전하고 싶은 부분이 있으면 도전해 봐도 좋겠습니다.
|
|
||
| import java.time.LocalDateTime; | ||
|
|
||
| public class EnrollmentApplication { |
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.
EnrollmentApplication 이름만으로 어떤 역할과 책임을 담당하고 있는지 파악하기 어려움
수강 신청 중인 수강생 정보를 담고 있는 것으로 보이는데 Student와 같은 이름으로 좀 더 명확히 하면 어떨까?
AI에게 이 객체가 담당하는 역할과 책임을 설명하고 적합한 이름을 찾아볼 것을 추천
| public class EnrollmentApplication { | ||
| private final Long sessionId; | ||
| private final Long nsUserId; | ||
| private final Payment payment; |
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.
굳이 금액 정보를 저장할 필요가 있을까?
| this.approvedBy = approvedBy; | ||
| } | ||
|
|
||
| public void approve(Long adminId) { |
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.
👍
| this.approvedBy = adminId; | ||
| } | ||
|
|
||
| public void cancel() { |
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.
👍
단, 승인 날짜와 승인한 사람의 정보를 기록하고 있는데 승인 취소에 대한 기록은 관리하지 않고 있다.
승인과 취소에 대한 정보를 관리하는 것이 중요하다면 이 테이블에 관리하기 보다 별도의 History 테이블을 통해 관리하는 것은 어떨까?
|
|
||
| List<EnrolledStudent> findBySessionId(Long sessionId); | ||
|
|
||
| void saveApplication(EnrollmentApplication application); |
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.
이미 EnrolledStudent가 있다면 EnrollmentApplication을 추가하는 대신 EnrolledStudent 객체를 리팩터링하는 것이 낫지 않을까?
| foreign key (image_id) references session_image (id) | ||
| ); | ||
|
|
||
| create table session_enrollment |
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.
안전하게 리팩터링하려면 기존의 session_enrollment에 alter table 구문을 통해 칼럼을 추가하는 방식으로 접근해야 점진적인 리팩터링이 가능해짐
안녕하세요 자바지기님.
미션 진행하다가 스트랭글러 무화과 패턴과 관련해서 진행방향이 맞는지 확인이 필요해서 모두진행하지않았지만 우선 PR올렸습니다.
무화과 패턴이 결국 기존 시스템을 죽이지 않고 조금씩 대체하며 점진적으로 새로운 시스템으로 개선을 하겠다는 건데 기존에 staus가 가지고있는 상태값을 포함한 ProgressStatus과 RecruitmentStatus로 분리하였는데 강의에 두 상태값을 추가하고나서 고민이 생겼습니다.
세션에 상태값들이 추가되면서 생각했던 방법은 3가지정도가 있는 것 같아요.
이 정도가 있을 것 같은데 처음 생각했던 방법은 스키마도 변경하고 객체에도 현재 PR시점에 상태인 status , ProgressStatus, Recruitmentstatus를 모두 존재하도록 진행하고 이후에 기존 DB데이터를 마이그레이션하여 새 컬럼으로 변환하고 그이후 Session에서도 Status를 제거하는 방식으로 진행하려 하는데 진행 방법에 대해 어떻게 생각하시나요?
프로그램 요구사항의 마지막부분때문에 데이터 변경이나 스키마 변경을 하지않고 진행해야하는건가 고민하면서 생각하게되었습니다.