🚀 4단계 - 수강신청(요구사항 변경) #770
Conversation
이렇게 구현하신 부분이 있나요? 정확히 어떤 부분을 말씀하시는지 제가 잘 모르겠어서요~ |
| public int getFindByUserIdCalled() { | ||
| return findByUserIdCalled; | ||
| } | ||
|
|
||
| public int getFindByUserIdsCalled() { | ||
| return findByUserIdsCalled; | ||
| } | ||
|
|
||
| public int getSaveCalled() { | ||
| return saveCalled; | ||
| } |
There was a problem hiding this comment.
사용하지 않는 메서드는 제거해주셔도 좋을것 같습니다.
fake 객체도 관리의 대상이다보니 가능한 관리포인트를 줄이는게 좋을것 같습니다. :)
| @Component | ||
| public class SessionImageFactory { | ||
|
|
||
| private final ImageHandler imageHandler; |
There was a problem hiding this comment.
몇가지 질문을 드리고 싶은데요.
- Factory를 Component로 사용해야하는 이유
- ImageHandler는 특정 메서드에서만 사용되는데 이러면 응집도가 떨어져 보이는데 어떻게 생각하시는지
- 모든 변환은 Factory를 반드시 사용하도록 컨벤션을 정하시고 진행하고 계신것인지
궁금하네요~
There was a problem hiding this comment.
2번 부분
초기 구현시 이미지 다운로드 기능을 전략패턴으로 해서 테스트 유연성을 가져가기 위함이였는데요
지금 구현된 것을 보니깐 해당 부분은 복잡도를 높이는 것 같아 수정했습니다.
height(), width(), byteSize() 메서드들을 구현하고 Test시에는 테스트용 클래스에 height(), width(), byteSize() 해당 메서드들을 오버라이드 해서 테스트를 유연한 구조로 만들었습니다.
3번 부분
네 맞습니다. 이번 프로젝트에서 해당 컨벤션으로 진행했습니다.
제가 PR 맨 처음 코멘트 에서 Factory에 대해서 여쭤 봤었는데 해당 코멘트 대로 정석적으로 가면 좋은 코드가 나올까 궁금하기도 해서 이번 기회에 한번 해봤습니다.
다만, 점점 불어나는 코드 양에 이게 괜찮을까..? 생각이 들었고요
#770 (comment) => 그래서 이 코멘트 부분에서 유연하게 사고해서 상황에 맞게 구현을 하고 여러 기법들은 썻을때 복잡도가 안 증가하면서도 유연하게 수정하는 코드를 만드는 것이 좋겠구나 생각했습니다.
또 한편으로는 말씀 그대로 만약 현업에서 프로젝트 받았을 때 저렇게 Factory로 만드는 컨벤션이 있다면 따르는게 맞을까 생각 들기도 하네요...(아직도 확실히 뭐가 좋은지는 헷갈리네요...ㅎㅎ)
1번 부분
3번 부분 답변에 이어서 말씀 드리면 우선 Factory를 Service가 의존성 주입을 받아야 하는 것 같아 Component로 뒀습니다.
만약 의존성을 주입받지 않으면 테스트 코드에서도 테스트용 인스턴스 대신 실제 Factory를 사용하게 되는데
Service 테스트코드는 Service 코드 자체의 로직만 테스트 해야지 Factory 코드의 동작에 영향을 받으면 안되지 않나 싶었습니다.
코멘트 주신대로 한번 Factory를 Component를 빼고 Service에서 그대로 설정해서 사용
ex. private final CourseFactory courseFactory = new CourseFactory();
이런 식으로 설정하는 것이겠죠?
이러면 서비스 코드에서 Factory 코드로 인해 오류가 나는 경우가 있어 아까 제가 말씀드린 "Service 테스트코드는 Service 코드 자체의 로직만 테스트"가 아닌 것 같아 주입받기 위해 우선 뒀습니다.
하지만, 서비스코드에 주입이 항상 Factory와 Repository가 들어가서 복잡도는 높아지는 것 같아 막 좋다고는 또 못할 거 같긴 하고요.
한번 1번 부분과 3번 질문 주신 의도와 생각을 여쭤봅니다.
감사합니다. 🙇♂️
There was a problem hiding this comment.
1번은 제가 어떤 리뷰의 방향성을 가지고 질문드린건 아니었구요 진짜 어떠한 의사결정 과정을 거치셨는지 궁금해서 질문드린부분이었습니다~
그런데 상태를 가지지 않는다면 그냥 static한 function으로 관리해도 괜찮지 않을까했습니다~
| } | ||
|
|
||
| public Session create(SessionEntity sessionEntity) throws IOException { | ||
| public Session createSession(SessionEntity sessionEntity, List<SessionImageEntity> sessionImageEntities) throws IOException { |
There was a problem hiding this comment.
Domain <-> Factory <-> Entity => 이 부분은 Domain이나 Entity 두 클래스 다 서로의 의존성을 가지기에는 Domain은 하는 일이 많고 Entity 클래스는 용도가 애초에 DB와 1:1 매핑을 하는 클래스인데 또 역할을 주는 것은 아닌 것같은데
이런 생각이 들어서 이 부분에 대해서 한번 의견을 여쭤봅니다.
이 부분 정답은 딱히 없는 것 같은데요
이렇게 도메인 객체 생성에 여러개의 협력 객체(엔티티)가 필요하다면 factory가 좋은 선택이 될 수 있다고 생각합니다.
하지만 엔티티 객체를 단순하게 도메인 객체로 변환이 가능하다면 저는 굳이 factory를 만들지 않고 entity에게 domain 객체로 생성의 책임을 부여하기도 합니다. 말씀하신대로 entity는 db와 밀접한 책임을 가지기에 이러한 부분을 엄밀히 따지면 어색해보이지만 편의성 자체도 무시하지는 못하는것 같아서 저와 제가 속한 팀에서는 이렇게 사용하기도 합니다 :) 제가 있는 팀에서 종종 그런 얘기를 하거든요. "우리는 예술 작품을 만드는게 아니잖아요."
There was a problem hiding this comment.
넵 "우리는 예술 작품을 만드는게 아니잖아요." 이 말이 많이 와닿네요.
이 부분은 구현 때마다 판단을 잘해서 유연하게 생각하고 구현 해야 겠네요 ㅎㅎ
| @Transactional | ||
| public boolean approve(long paymentId, String approverId) throws IOException { |
There was a problem hiding this comment.
이 부분은 BaseCode에 객체 필드값만 바꾸길래 JPA 동작처럼 구현하면 되나 했는데 역시 좀 부족했군요!
이 부분은 repository에 delete 로직 추가했고 service코드의 삭제 로직은 일부 리펙토링 했습니다.
이 부분은 DTO는 해당 프로젝트에는 없는데요! DTO나 Entity는 역할이 조금 다르지만 결국 다른 곳에서 데이터를 가져오거나 보낼때 쓰이는 클래스로 알고 있고 Domain 클래스가 있다 보니 DTO나 Entity 이 두 클래스와 Domain 사이에 변환이 자주 일어나고 그 변환은 Factory 클래스가 담당하는 구조로 생각하고 여쭤봤습니다~! DTO 부분도 뭔가 #770 (comment) 해당 코멘트가 답이 될 수도 있다고 생각이 드네요 DTO 에서 Domain으로 전환하는 메서드가 DTO에 있을수도 혹은 복잡도가 있다면 Factory 클래스처럼 새로 만들던가 하는 것 처럼 비슷하게 생각했는데 찬인님이 생각하는 것과 제가 비슷하게 생각하고 있는 거겠죠...? |
|
코멘트 반영했습니다! 꼼꼼하게 리뷰해주셔서 감사합니다~! |
| public void deleteSession(long sessionId) throws IOException { | ||
| Session session = sessionFactory.create(sessionRepository.findById(sessionId)); | ||
| session.delete(); | ||
| public void deleteSessions(long courseId) { |
|
|
||
| public SessionService(SessionRepository sessionRepository, SessionFactory sessionFactory) { | ||
| public SessionService( |
There was a problem hiding this comment.
h2 의존성이 이 프로젝트에 존재하기 때문에 테스트에서 h2 db를 이용한 jdbcRepository의 테스트가 가능할 것 같습니다.
지금 몇몇 서비스와 repository의 테스트가 없는것 같은데요~ 이 부분도 함께 추가해줄까요?
fake 객체를 사용하면 단위테스트가 빠르게 동작할 수 있지만
통합테스트도 없으면 안되는 중요한 요소로 볼 수 있기 때문입니다~
혹시 책 '단위 테스트' 읽어보시지 않았다면 추천드립니다~
맞습니다~! |
|
늦게 다시 피드백 반영 해서 드리네요! 통합 테스트는 제가 익숙하지 않아서 특정 메서드를 테스트 하기 위해 다른 메서드들까지 많이 사용한 것 아닌가 생각이 들긴 했네요..ㅎㅎ 결국, Fake 객체를 활용한 기능 테스트는 제가 정의한 비지니스 로직대로 잘 흘러가는지를 확인 함을 이번기회에 느꼈고, 감사합니다~! 🙇♂️ |
Step4 마지막 리뷰 잘 부탁 드립니다.
요구사항 만족 및 전반적인 코드 리펙토링을 했는데 코드 수정이 너무... 많네요
코드 제거 및 로직들을 분리하고 스타일 맞춘다고 코드 양이 많아진 것 같아요 죄송합니다. ㅠㅠ
항상 많은 코드양에도 리뷰 꼼꼼하게 주셔서 감사합니다.
#750 (review)
해당 코멘트도 수정해서 같이 반영 했습니다.
#750 (comment)
여기에서 말씀 주신대로 fake 객체를 TestUserRepository 여기에 비슷하게 적용해봤는데 좋은 것 같아 앞으로도 좀 더 발전시켜서 잘 써먹어야 겠네요 😄 감사합니다. 🙇♂️
전반적으로 Entity <-> Domian간의 생성을 Factory 클래스로 하다보니 Service 코드에 Repository, Factory 의존성이 많이 들어가는 것 같아서 잘 하고 있나..? 생각이 들었습니다.
(Service에서 Repository에서 Entity와 Domain으로 왔다갔다 하면서 로직들이 구성되니 결국 Service 코드에 이렇게 되는 것 같아서요)
그래서, 제가 생각할 때는 Domain 객체로 DB 정보를 받아올 수 있으면 받아오되 Domain 객체가 너무 복잡하면 Entity 클래스를 별도로 만들고 Factory 클래스를 만들어서 의존 관계를 분리하는 것이 대체적으로 안정적이겠다 라고 생각이 들긴 했는데요
하지만, 그러면 항상 아래 구조가 안정적인가?
DTO <-> Factory <-> Domain <-> Factory <-> Entity
이러면 너무 복잡도가 증가되는 것 아닌지 조금 궁금했고
DTO <-> Factory <-> Domain => 이 부분은 DTO가 변동사항이 많으니 Factory가 너무 무분별하게 있으면 DTO가 Domain을 가져도 될 것 같기는한데
Domain <-> Factory <-> Entity => 이 부분은 Domain이나 Entity 두 클래스 다 서로의 의존성을 가지기에는 Domain은 하는 일이 많고 Entity 클래스는 용도가 애초에 DB와 1:1 매핑을 하는 클래스인데 또 역할을 주는 것은 아닌 것같은데
이런 생각이 들어서 이 부분에 대해서 한번 의견을 여쭤봅니다.
항상 적용되는 법칙은 없겠지만 🤔 그래도 이번 과정 미션을 통해서 전반적으로 객체 의존관계가 매번 고민이 되는 거라서 한번 여쭤봅니다.
감사합니다.