수강신청 - 레거시 코드 리팩터링 회고

 

강사님이 가장 중요한 미션이라고 여긴 레거시 코드 리팩터링 미션이다.

앞의 미션들은 이 미션을 위한 연습이라고 해도 과언이 아닐 정도라고 하셨다.

앞의 미션들은 하지 않아도 레거시 코드 리팩터링 미션은 무조껀 해야 비싼 강의에서 얻어갈 수 있는 것이 있다고 말씀하셨다.

 

이전 콘솔 기반 미션과는 많이 다르고 Base 코드도 있다보니 코드를 어디까지 수정해도 되는지 감이 잘 잡히지 않았다.
예를 들어 Answer과 Question 객체에서 양방향 관계로 되어 있는걸 해제해도 되는지 등이다.

Step1 1단계 레거시 코드 리팩터링

Step1 1단계 레거시 코드 리팩터링

 

Step1 미션은 Service Layer에 집중되어 있는 로직을 도메인 모델로 옮기는 미션이였다.

객체지향 적으로 코드를 작성하지 않으면 Service에 모든 로직이 집중 되어 있고, 이를 Fat Service 라고 한다.

Service Layer는 infar와 도메인 간의 중간 역할을 수행하면서, DB 조회 및 저장, 트랜잭션 관리 등을 하는게 역할상 알맞다.

 

미션 때 마다 기능 목록을 제일 먼저 진행한다

* [X] 질문을 삭제할 때 답변 또한 삭제 해야 하며, 답변의 삭제 또한 삭제 상태(deleted)를 변경 한다.
* [X] 질문과 답변 삭제 이력에 대한 정보를 DeleteHistory를 활용해 남긴다.

 

아래와 같은 피드백을 받았다.

💡 기능 목록 작성👍
객체별로 기능을 정리해 보셔도 좋을 것 같아요

 

- [ ] 질문
  - [ ] soft delete
  - [ ] 작성자만 삭제 가능
  - [ ] ...
- [ ] 답변
  - [ ] soft delete
  - [ ] ...

 

 

테스트 에서만 사용하는 생성자를 만들었다.

  public Answer(NsUser writer, Question question) {
        this(null, writer, question, null);
    }

 

아래와 같은 피드백을 받았다.

💡 테스트에서만 사용되는 생성자네요
생성자를 추가하는 것도 좋지만 해당 테스트 클래스 내에서 기존 생성자를 활용한 메서드를 추가해 보셔도 좋을 것 같아요

 

@Test
void test() {
    Answer answer = javajigiAnswer(Q1);
    ...
}

public Answer javajigiAnswer(Question question) {
    return new Answer(JAVAJIGI, question, null);
}

 

해당 테스트 클래스 내에서 만들면 메소드 명으로 테스트에 필요한 데이터가 어떤 것인지 의미를 부여할 수 있어 더 좋은 방법인 것 같다.

 

public class Answer {

    //...

    public void delete() {
        this.deleted = true;
    }

 

Answer를 삭제하는 로직이 있었다.

요구사항 중에 질문은 작성자만 삭제할 수 있는 로직이 있었다.

 

아래와 같은 피드백을 받았다.

💡 답변 삭제에 대한 테스트가 없네요 😢
답변은 작성자와 일치하지 않아도 삭제 가능한가요?

 

질문 삭제시에 대한 예외처리는 했지만 질문과 영속된 답변 삭제에 대한 예외처리는 체크를 하지 못했다.

단일 객체 관점에서도 예외를 체크하는게 중요한 걸 깨달았다.

 

    public boolean isAllSameBy(NsUser questionWriter) {
        if (hasNoAnswers()) {
            return false;
        }
        return answers.stream()
                .allMatch(answer -> answer.isOwner(questionWriter));
    }

 

답변 객체를 순회하면서 질문 작성자와 일치하는지 검증하는 로직이다.

 

아래와 같은 피드백을 받았다.

💡 일급 컬렉션 👍
질문을 삭제할 때에만 사용된다는 보장은 없으니 인자의 이름을 writer 로 변경하면 어떨까요?

이 피드백도 객체 관점에서 일반적인 명을 사용하는게 코드 가독성에 좋아서 이러한 피드백을 주신 것 같다.

 

 

    public void delete(NsUser writer) throws CannotDeleteException {
        if (this.writer != writer) {
            throw new CannotDeleteException("다른 사람이 쓴 답변이 있어 삭제할 수 없습니다.");
        }
        this.deleted = true;
    }

 

아래와 같은 피드백을 받았다.

💡 Answer 입장에서 다른 사람이 쓴 답변에 대해 알 수 없을 것 같은데요
예외 메시지가 적절한지 고려해 보시면 좋을 것 같아요

 

예외 메시지도 마찬가지이다!!

 

 

  @Transactional
    public void deleteQuestion(NsUser loginUser, long questionId) throws CannotDeleteException {
        Question question = questionRepository.findById(questionId).orElseThrow(NotFoundException::new);
        question.delete(loginUser);
        List<DeleteHistory> deleteHistories = question.deleteHistory();

 

QuestionService#deleteQuestion에서 Question#delete와 Question#deleteHIstotry를 호출하고 있었다.

 

아래와 같은 피드백을 받았다.

💡 Question#delete 메서드를 호출하지 않고 question#deleteHIstory를 호출하면 실제 삭제 일시와 상관없는 DeleteHistory 객체가 생성됩니다
혹은 삭제 상태가 아니더라도 DeleteHistory 를 반환할 수 있는데요
삭제와 동시에 삭제 이력을 반환하면 어떨까요?

 

뼈를 때리면 말씀이셨다.

Question#delete에서 questionHistory 객체까지 생성 후 반환 하도록 수정하였다.

    @Transactional
    public void deleteQuestion(NsUser loginUser, long questionId) throws CannotDeleteException {
        Question question = questionRepository.findById(questionId).orElseThrow(NotFoundException::new);
        List<DeleteHistory> deleteHistories = question.delete(loginUser);
        deleteHistoryService.saveAll(deleteHistories);
    }

 

 

    @Test
    @DisplayName("실패 - 답변 작성자와 일치하지 않을 경우 답변을 삭제 하지 못한다.")
    void success_delete_answer_writer_answer_writer_same() throws Exception {
        Assertions.assertThatThrownBy(() -> A1.delete(SANJIGI))
                .isInstanceOf(CannotDeleteException.class)
                .hasMessage("다른 사람이 쓴 답변이 있어 삭제할 수 없습니다.");
    }

 

답변 작성자와 일치하지 않을 경우 답변을 삭제할 수 없는 실패 테스트 코드이다.

 

아래와 같은 피드백을 받았다.

💡 DisplayName과 예외 메시지가 일치 하지 않아서 혼동이 생길 수 있을 것 같아요
기능의 의도와 맞는 메시지로 변경하면 좋을 것 같아요

 

테스트 하려는 사항과 예외 메시지를 통일시켰다.

 

  deleteHistoryService.saveAll(question.delete(loginUser));

 

Qustion#delete 에서 DeleteHIstory를 반환해서 메소드를 saveAll 인자에 바로 적용하였다.

 

아래와 같은 피드백을 받았다.

       final List<DeleteHistory> deleteHistories = question.delete(loginUser);
        deleteHistoryService.saveAll(deleteHistories);

💡 지역 변수를 선언하면 삭제 이력을 남긴다는 의도가 잘 드러날 것 같은데 어떠신가요?
디버깅에도 용이할 것 같아요

문맥에 따라 지역변수를 선언하여 코드 가독성을 판단 하면 되는데, 내 생각은 deleteHistoryService.saveAll 자체로 이력을 남긴다는 의도가 들어나는 것으로 생각된다.

 

 

Step2 수강신청(도메인 모델)

Step2 수강신청(도메인 모델)

 

요구사항에 맞게 도메인 모델을 설계하는 미션이였다.

Step3 단계가 디비연동 이기 때문에 디비는 제외하고 진행을 하였다.

 

 

 

  public void register(Payment payment) {
        validateState();
        validateType();
        validatePriceEqualPayment(payment);
        sessionUserCount.plusUserCount();
    }

 

수강신청시 필요한 에외처리를 검사하고 있었다.

 

아래와 같은 피드백을 받았다.

💡 수강생 '수'만 관리하고 있어서 중복 신청 방지 기능은 도메인에서 해결하지 못할 것 같은데요
최대 인원, 모집 상태 처럼 중복 신청도 도메인에서 방지하면 어떨까요?

 

public class Session {
    private final SessionUserCount 

public class SessionUserCount {
    private int userCount;
    private final int maxUserCount;

 

현재는 수강신청 제한 인원만 검사하고 있어 중복 신청이 가능한 도메인 구조 였다.

 

아래와 같이 수강신청한 인원들에 대한 일급 컬렉션을 만들어 중복 신청 로직을 구현하였다.

public class SessionUsers {
    private Set<SessionUser> sessionUsers;
    
   public SessionUser addUser(long sessionId, SelectionStatus selectionStatus, NsUser user) {
        SessionUser sessionUser = new SessionUser(sessionId, user.getId(), selectionStatus);
        validateDuplicateUser(sessionUser);
        sessionUsers.add(sessionUser);
        return sessionUser;
    }

    public void validateDuplicateUser(SessionUser sessionUser) {
        if (sessionUsers.contains(sessionUser)) {
            throw new SessionUserException("강의를 이미 신청한 유저이므로 중복으로 신청할 수 없습니다.");
        }
    }

 

 

    private boolean isOpen() {
        return sessionState == SessionState.OPEN;
    }

 

Session 객체에서 상태값을 검증하여 boolean값을 반환하는 로직이다.

 

아래와 같은 피드백을 받았다.

    private boolean isOpen() {
        return sessionState.isOpen();
    }

💡 메시지를 보내도 좋을 것 같아요

 

아주 사소한 거라도 메시지를 보내도록 습관화 하자!!

 

    @ParameterizedTest
    @MethodSource("provideSessionStateExcludeOpen")
    
       private static Stream<Arguments> provideSessionStateExcludeOpen() {
        return Stream.of(
                Arguments.of(SessionState.PREPARE),
                Arguments.of(SessionState.CLOSE)
        );
    }

 

enum 테스트도 Stream<Argements> 를 만드는 방법으로 하고 있었다.

 

아래와 같은 피드백을 받았다.

💡 EnumSource를 활용할 수 있을 것 같아요

 

    @ParameterizedTest
    @EnumSource(value = SessionStatus.class, names = {"PREPARE", "PROGRESS", "COMPLETE"})
    @DisplayName("실패 - 수강 신청시 모집중이 아닌 경우 수강 신청을 할 수 없다.")
    void fail_session_register_not_open(SessionStatus sessionStatus) {

 

@EnumSource를 사용하면 메소드를 따로 만들지 않아도 된다.

생각해보니 단일값이라 메소드 추출 자체를 할 필요가 없었다.

 

 

Step3 수강신청(DB 적용)

Step3 수강신청(DB 적용)

 

DB를 연동하는 과정에서 전 단계들보다 많이 헤멨다.

Jdbc를 사용하면서 Step2에서 설꼐한 도메인 모델을 해치지 않는 선에서 개발을 맞춰야 했다.

그리고 DB까지 들어가니 익숙치가 않았다.

아마 여태 미션 과정들 중에서 제일 고통스러웠다.

 

DB 테이블도 설계를 했어야 해서 mermaid를 사용해서 erDiagram를 만들었다.

가볍게 쓰기에 좋아서 추천한다. Jetbrains 툴의 플러그인에도 있다.

https://mermaid.js.org/

 

 

  public Session(Long id) {
        this(id, null, null, null, null, null, null, null, null);
    }

테스트시 id 값 할당이 필요해서 만든 생성자이다.

 

아래와 같은 피드백을 받았다.

💡 테스트에서만 사용되는 생성자네요 😃
null 초기화보단 기본 값을 할당해줘도 좋을 것 같아요

 

null은 지양 하고 기본값으로 할당해 주었다.

 

public class Session {

    private Long id;

    private Long courseId;

    private final SessionImage sessionImage;

    private final SessionPeriod sessionPeriod;

    private final SessionPrice sessionPrice;

    private final SessionState sessionState;

    private final SessionType sessionType;

    private final SessionUsers sessionUsers;

    private final SessionUserCount sessionUserCount;
    
    //..
    
    public void register(NsUser user, Payment payment) {
        validateState();
        validateType();
        validatePriceEqualPayment(payment);
        sessionUsers.addUser(user);
        sessionUserCount.plusUserCount();
    }

 

강의 세션 객체에 있는 상태 필드들이다.

 

아래와 같은 피드백을 받았다.

💡 기능이 추가되거나 변경되면 필드도 추가되고 메서드 내부도 계속 늘어날 것 같은데요
수강신청 객체를 도출해서 관련 책임들을 모두 위임하는 구조도 고려해 보시면 좋을 것 같아요

 

 public void register(NsUser user, Payment payment) {
        enrollment.enroll(user, payment);
    }

 

SessionImage와 SessionPeriod를 제외하고는 수강 신청과 관련된 객체들이다.

그래서 Enrollment 객체를 만들어서 필드들과 관련된 로직들을 옮겼다.

그리고 Session은 Enrollment 객체를 주입받아 메소드 호출을 위임하였다.

 

몇가지 궁금한게 있어서 아래와 같은 질문을 하였다.

assertThat(users).hasSize(2)
                .extracting("id")
                .containsExactly(1L, 2L);

 

repository 테스트 코드 검증시 조회한 도메인 모델의 데이터를 모두 getter로 꺼내서 검증 해야 할까요? 위와 같은 필요성을 느끼질 못해서 id 혹은 테스트와 관련된 데이터만 검증했습니다..!

 

아래와 같은 피드백을 받았다.

💡 지금처럼 필요한 값 위주로 검증해 주셔도 될 것 같아요 😃

 

 

또 하나의 질문을 하였다.

  @Transactional
    public void register(long sessionId, NsUser user, Payment payment) {
        SessionUsers sessionUsers = sessionUsersRepository.findBy(sessionId);
        Session session = sessionRepository.findBy(sessionId, sessionUsers)

조회한 SessionUsers를 Session 생성과 같은 생명 주기로 생성하고 싶었습니다만.. 위와 같은 코드로 구현을 하긴 했는데 도저히 아닌 것 같지만 올바른 방법이 떠오르질 않네요 ㅠㅠ JPA를 사용시 일급 컬렉션을 많이 쓰던데 그건 어떻게 가져오는건지 참..

 

서비스 레이어에서 sessionUsers 데이터를 별도 조회 한다음 해당 객체를 sessionRepository.findById에 넘겨준다.

넘겨주는 이유는 Session 객체 생성시 SessionUsers 객체도 필요하기 때문이고, 궁극적인 이유는 도메인 설계를 유지하기 위함이다.

 

아래와 같은 피드백을 받았다.

💡 session 조회 시 sessionUsers를 모두 포함하는 형태를 원하셨다면
sessionRepository 내부에서 sessionUserRepository를 활용해 보셔도 될 것 같아요

 

public class JdbcSessionRepository implements SessionRepository {

    private final SessionUserRepository sessionUserRepository;
    ...
    public Optional<Session> findBy(long sessionId) {
        final SessionUsers sessionUsers = sessionUsersRepository.findBy(sessionId);
        ...
    }

}

 

처음에 위방법으로 했다가 안됬다. 안된 이유는 테스트 코드에서 SessionUsersRepository를 주입 받지 못했다.

하지만 다시 서치해보니 @import({JdbcSessionUsersRepository.class})  를 사용하면 주입 받을 수 있었다..!!

 

 

※ JPA를 사용하면 이렇게 까지 번거롭게 할 필요는 없을 것 같다. 다만 양방향 연관 관계를 맺어야 JPA 일급 컬렉션을 적용하기 편할 것 같은 느낌이 든다. 연관 관계를 맺지 않으면 조회 로직을 직접 짜야 한다.

개인적으로 OneToMany 사용을 지양한다.

Many 쪽의 컬렉션을 참조해서 가져올때 예상치 못한 쿼리가 몇번 나간적이 있어서 그렇다.

사이드 프로젝트를 통해서 시도를 해봐야 겠다.

 

 

Step4 수강신청(요구사항 변경)

Step4 수강신청(요구사항 변경)

 

하나 궁금한 사항이 있어 질문을 하였다.

 

💡 모집 상태는 종류가 두가지(비모집중, 모집중)로 명확한 느낌이 들어서 디비 필드를 "Y", "N"으로 할까 하다가 문자열(OPEN, CLOSE)로 관리하는걸 선택했는데요.모집 상태가 아니더라도 값 성격상 종류가 두가지로 명확하면 is_ OO("Y", "N")와 같이 하는게 좋을까요? 아니면 그래도 문자열 값으로 관리 하는게 좋을까요? 리뷰어님의 생각이 궁금합니다~

 

아래와 같은 피드백을 받았다.

💡 가능하다면 의미를 드러낼 수 있는 단어를 사용하는 것을 선호합니다 😃
필드명이나 엔티티명 등 부가 정보로 Y/N 의 의미를 유추할 수도 있지만 100% 보장되지 않는다고 생각해요

 

 

강사 개념이 생겨났고, 조건에 따라 수강 승인, 취소할 수 있는 요구사항이 생겼다.

대략적으로 아래와 같이 구현하였다.

    @Transactional
    public void approve(long sessionId, NsTutor tutor, NsUser user) {
        Session session = sessionRepository.findBy(sessionId)
                .orElseThrow(() -> new SessionException("강의 정보를 찾을 수 없습니다."));
        SessionUserStatus approve = tutor.approve(session, user);
        sessionUsersRepository.updateSessionUserStatus(sessionId, user.getId(), approve);
    }

 

    public SessionUserStatus approve(Session session, NsUser nsUser) {
        if (!session.isTutor(this.id)) {
            throw new SessionApproveException("본인 담당 강의만 수강 승인 할 수 있습니다.");
        }

        if (!nsUser.isSelected()) {
            throw new SessionApproveException("선발 되지 않은 유저는 수강 승인 할 수 없습니다.");
        }
        return SessionUserStatus.APPROVE;
    }

 

강사가 승인 단계에서 예외처리가 발생하지 않으면 수강 승인 상태값만을 반환한다.

 

아래와 같은 피드백을 받았다.

💡승인 기능은  수강신청한 사람 의 상태를 변경하는 기능이니 수강신청한 사람들을 알고 있는 객체에게 메시지를 보내서 승인된 수강신청한 사람 객체를 반환 받는 로직도 고려해 보시면 좋을 것 같아요

승인 기능은 상태를 변경하는 기능인데 변경 없이 유효성 검증만 하고 상태 값만 반환하는게 객체의 책임으로 적절한지 고민해 보시면 좋을 것 같아요.

 

수강신청한 사람을 알고 있는 객체는 Session이다. 튜터가 approve하는게 아닌 Session이 가지고 있는 상태값을 이용해서 위임을 하여 강의를 승인 할 수 있는 상태인지 검사하는 역할을 부여하면 되었다.

아래와 같이 수정하였다. 호출 순서는 위에서 아래이다.

 

    @Transactional
    public void approve(long sessionId, NsTutor tutor, NsUser user) {
        Session session = sessionRepository.findBy(sessionId)
                .orElseThrow(() -> new SessionException("강의 정보를 찾을 수 없습니다."));

        SessionUser sessionUser = session.approve(tutor, user);
        sessionUsersRepository.save(sessionUser);
    }

   // ==

    public SessionUser approve(NsTutor tutor, NsUser user) {
        tutor.isSameTutor(this.tutorId);
        return enrollment.approve(user);
    }

   // ==

    public SessionUser approve(NsUser user) {
        return sessionUsers.approve(user);
    }

   // ==

    public SessionUser approve(NsUser user) {
        return modifyUser(user, SessionUser::approve);
    }

    private SessionUser modifyUser(NsUser user, Function<SessionUser, SessionUser> operation) {
        return sessionUsers.stream()
                .filter(sessionUser -> sessionUser.isSameUser(user))
                .map(operation)
                .findFirst()
                .orElseThrow(() -> new SessionUserException("강의를 신청 하지 않은 유저입니다."));
    }