[레거시 코드 리팩터링 - 1단계] 유콩(김유빈) 미션 제출합니다. #269
Conversation
| public static Product 후라이드_치킨의_가격은(final BigDecimal price) { | ||
| return 상품() | ||
| .이름("후라이드 치킨") | ||
| .가격(price) | ||
| .build(); | ||
| } | ||
|
|
||
| private static ProductFixture 상품() { | ||
| return new ProductFixture(); | ||
| } |
There was a problem hiding this comment.
test fixture 꼼꼼하게 작성해주셨네요 한글로 작성하셔서 가독성 엄청 좋네요 👍🏻
피드백에 앞서서 제가 드리는 내용은 지극히 주관적이니, 참고만해주세요
저는 현재 사용하고 있는 테스트 객체가 변경될 여지가 있을 때 (update 를 검증하거나, jpa 에 eneity 로 사용된다거나... )
객체를 새로 생성해서 반환하는데요. 현재 로직을 Fake 로 구현해주신것도 그렇고, Fixture 의 형태도 그렇고 JPA 로 변경을 염두해두신걸까요 ?
근본적으로 왜 리팩터링을 해야할까요? 왜 모든 코드는 레거시가 되는 걸까요? ㅠㅠ
이질문에 대한 제 생각은 그때의 상황에서는 맞았지만, 변화한 지금에는 개선이 필요해졌기 때문, 코드가 변하는건 너무 당연하는 순리를 깨달았고, 유연한 변화를 위해 객체지향 설계와 리팩터링에 대한 지속적인 고민이 필요하다! 라고 정의 내렸습니다.
저는 리팩터링 미션이 주는 메세지가 현업에서 마주할 레거시를 대하는 나만의 자세를 구축하라. 라고 생각해요
그래서 미래에 있을 변화보다 현재의 집중한 최소한의 리팩터링을 하고, 그 이후에 정책이나, 사용 기술이 바뀔 때 현재 설계에서
어떤 점 때문에 불편함이 있었는지 생생히 기억하고 그걸 반복하지 않기 위한 다음 스텝에 대한 고민을 하고 싶어서 이번 미션에서 목킹을 사용했습니다.
요약!
이 피드백에서 드리고 싶은 내용은 리팩터링이란 무엇일까? 생각해보시고, 유콩만의 정의를 내려보면 좋겠다!
There was a problem hiding this comment.
리팩터링 미션의 목적
저는 이번 미션의 목적이 프로젝트를 시작할 때의 도메인 파악하는 방법 익히기 와 점진적으로 레거시 코드를 리팩토링 이라고 생각했어요. 그리고 1단계가 도메인을 파악하기 위해서 테스트 코드와 요구사항 목록을 작성했다고 이해했습니다. 그러다보니 이번 1단계에서는 확장을 고려하기 보다는 현재의 코드를 파악하는데에 목적을 뒀어요. (아직 JPA 는 생각하지 않았습니다...!!)
가짜 객체를 사용한 이유는 아래의 다른 피드백에 적어두었고, Fixture 는 사실 제가 작성한 테스트 코드를 이해하기 어려워서 만들었습니다. 😅 수달에게 제출한 코드를 다시 수정하고 싶다고 요청하기 전의 코드에는, 모든 객체를 테스트 내부에서 직접 생성하고 활용했는데요, 그러다보니 현재 테스트하고 싶은 것이 무엇인지 제가 헷갈리더라구요. 레거시 코드를 리팩토링 하기 위해서 작성하는 코드인데 오히려 레거시 코드를 탄생시켰습니다..
리팩터링에 관해서
리팩터링을 해야하는 근본적인 이유에 대해서 물어보신 걸까요? 모든 코드는 레거시가 된다, 라는 말에 동감합니다. 레거시가 되는 이유는 다양할거라고 생각해요. 그 당시 작성하는 개발자의 지식 수준, 지속적으로 업데이트 되는 언어와 프레임워크, 시시각각 변화하는 요구사항 등. 가만히만 있어도 레거시 코드가 되는 환경에서, 현재 제공하는 기능들을 훼손시키지 않고 단계적으로 리팩터링 하는 과정이 필수가 되지 않을까요?
또한 저도 발생할 가능성이 적은 아주 미래의 일까지 고려하는 코드를 작성하기 보다 현재에 집중하되, 기능이 확장될 가능성이 있는 부분에 한해서만 대비를 하는 것이 좋다고 생각해요. 확장을 고려한 코드는 진짜로 확장할 가능성이 있을 경우에는 활용하기에 아주 좋지만 그렇지 않다면 코드를 이해하기 어렵게 만들 뿐이니까요.
나름 열심히 답변해봤는데 수달이 묻고 싶었던 내용이 맞을지는 잘 모르겠네요.. 🥹
|
|
||
| public class FakeMenuGroupDao implements MenuGroupDao { | ||
|
|
||
| private long id = 0L; |
There was a problem hiding this comment.
현재 DB 를 흉내내는 Map 은 참조형 타입을 사용하고, 지역변수로 선언된 id 는 기본형 타입을 사용하고 있네요
위 두가지의 차이가 무엇일까요 ? 연산작업, 캐스팅, 불변객체, 트레이드오프 키워드에 대해서 생각해보시고 유콩의 의견을 묻고 싶어요 !
There was a problem hiding this comment.
id 필드는 DB 가 관리하는 값이므로 null 허용을 열어줄 필요가 없다고 생각해 기본형 타입을 사용했고, Map 으로 감싸기 위해 참조형 타입을 사용했습니다!
불변객체 는 Long 타입이 불변을 유지한다고 해서 나온 키워드일까요? 트레이드오프 키워드는 어떤 의미인지 파악하지 못했습니다ㅠㅠ
|
|
||
| @Override | ||
| public List<OrderLineItem> findAll() { | ||
| return List.copyOf(orderLineItems.values()); |
There was a problem hiding this comment.
List.copyOf 와 Arrays.asList, new 로 객체를 생성하는 것에 차이는 무엇일까요 ? 여기에는 왜 List.copyOf 를 사용했나요 ?
There was a problem hiding this comment.
List.copyOf 를 사용한 이유
저는 보통 객체 내부에서 관리하는 컬렉션을 외부로 반환할 때 다음과 같은 선택지 중에서 선택합니다.
- 내부에서 사용하는 값 그대로 반환 : 외부 영향을 그대로 받으므로 거의 사용하지 않음
- new : 객체의 값을 사용하여 새로운 컬렉션이 필요한 경우
- List.copyOf : List 를 그대로 복사하되 수정할 수 없음
여기에서는 테스트에서 사용하기 위한 가짜 객체일 뿐이지만 테스트에서 수정할 가능성을 없애기 위해 List.copyOf 를 사용했어요.
Arrays.asList
Arrays.asList 의 경우는 자세하게 찾아본 적이 없었는데 찾아보니 고정된 크기의 컬렉션을 생성한다는군요. 이것도 List.copyOf 처럼 변경을 막는 방법으로 활용할 수는 있겠으나, 테스트 해보니 set 은 그대로 사용이 되네요. 결국 크기만 고정했을 뿐 내부의 변동까지는 막지 않았으니 앞으로도 불변으로 반환하고 싶다면 List.copyOf 를 사용하려고 합니다!
(크기를 고정시켜준다는 것은 너무 좋지만 이름이 직관적이지 않아 조금 아쉽네요 😢)
There was a problem hiding this comment.
저도 이 부분을 모르고 있었는데요, 알고 있으면 좋을 것 같아서 말씀드려봤어요!
| final Product product = 후라이드_치킨의_가격은(new BigDecimal(-1)); | ||
|
|
||
| assertThatThrownBy(() -> productService.create(product)) | ||
| .isInstanceOf(IllegalArgumentException.class); |
There was a problem hiding this comment.
IllegalArgumentException.clsss 로 전체 에러가 핸들링 되고 있는데, 위 에러를 받으면 어떤 도메인에서 어떤 문제가 있는지 파악하기 어려울 것 같아요. 😭
There was a problem hiding this comment.
넵.. 어렵기는 했습니다 다른 상황에서 터진 예외인데도 어디에서 발생한 예외인지 알 수가 없더라구요 😂
하지만 이번 1단계는 실제 코드를 리팩토링하는 것이 아닌 리팩토링을 하기 위해 도메인을 파악하고, 이후에 변경되었을 때 기존의 코드를 훼손시키지 않기 위해 테스트 를 작성하는 단계라고 이해했어요. 그래서 이번 단계에서는 최대한 실제 코드는 수정하지 않았습니다!
커스텀 예외와 메시지 자체는 크게 side effect 가 없을거같기는 한데, 프로젝트를 온전히 파악하지 않은 단계에서 제가 함부로 판단하지 않기 위해 추가하지 않았어요.
| - [x] 상품의 가격은 `null` 이 아니어야 한다. | ||
| - [x] 상품의 가격은 `0` 원 이상이어야 한다. | ||
| - [x] 상품의 목록을 조회할 수 있다. | ||
|
|
There was a problem hiding this comment.
혹시 상품명은 중복될 수 없다 와 관련된 로직 또는 제약조건이 어디에 있을까요...?? 🥺
There was a problem hiding this comment.
제약 조건은 명시되지 않았는데, 제가 생각하기에 필요하다고 느껴서 제안드려봤어요!
| @@ -6,7 +6,7 @@ plugins { | |||
|
|
|||
| group = 'camp.nextstep.edu' | |||
| version = '0.0.1-SNAPSHOT' | |||
| sourceCompatibility = '1.8' | |||
There was a problem hiding this comment.
자바 8 버전의 특징과 11버전의 특징은 무엇인가요 ? 11 버전의 기능을 사용하고 싶어서 올리셨나요 ? ㅎㅎㅎㅎ
There was a problem hiding this comment.
테스트 작성할때 제가 애용하는 List.of 를 사용하기 위해 추가했습니다.
There was a problem hiding this comment.
헉 갑자기 커멘트가 달려서 빈 내용으로 가버렸네요 ㅠㅠ
우테코 첫 미션과 끝 미션을 페어로, 리뷰어로 함께하게 되어서 감회가 새롭네요.
파이썬만 하다가 자바가 처음이라며 서툴수 있다고 걱정하던 유콩 모습은 더이상 보이지 않네요
처음 과 끝을 마주하고 있자니 유콩이 얼마나 성장했는지, 성장하기 위해서 노력했는지 느껴져서 괜히 아침부터 감동받았네요. 테스트 코드를 작성하는 방법에 대해서도 많은 고민을 한게 느껴졌어요. 특히 Fake 객체 만드시느라 엄청 고생하셨겠어요. ㅠㅠㅠ
제가 드리는 리뷰는 주로 의견을 묻는 형태로 진행되어있어요. 시간이 없으시다면 생략해도 좋습니다!
좋은 하루 보내세요 :)
| private Long id; | ||
| private String name; | ||
| private BigDecimal price; | ||
| private Long menuGroupId; | ||
| private List<MenuProduct> menuProducts; |
There was a problem hiding this comment.
실제 객체에 있는 필드를 사용하시고 계신거 같은데, MenuFixture 에 있는 필드에 용도는 어떤걸까요 ?
(제가 파악하지 못했을 수도 있어요 ㅠㅠ)
There was a problem hiding this comment.
처음에는 Fixture 안에 빌더 패턴을 활용하여 객체를 생성하는 로직만 존재했었는데요, 모든 테스트에 빌더 패턴을 사용한 객체 생성 로직이 존재하더라구요. 가독성을 높이기 위해서 선택한 결정이었는데 오히려 작성하는 코드 라인 수만 증가하고 가독성이 떨어져서(현재 테스트에서 강조하고 싶은 값이 잘 안보였어요) Fixtrue 의 역할을 바꾸어보았습니다. Fixture 내부에서 빌더 패턴을 활용하여 객체를 생성하고 특별한 값을 가지는 객체를 생성하는 로직이 존재하도록이요!
그 과정에서 빌더 패턴을 사용하기 위해 만든 메서드와 필드는 제거할까.. 싶었지만 Fixture 도 결국은 코드이기 때문에 가독성을 위해 그대로 두었습니다. 테스트마다 중복되는 빌더 패턴이 가독성을 떨어뜨렸을 뿐, 해당 로직을 Fixture 로 모아 일부에서만 사용하는 것과는 다르다고 생각했어요.
There was a problem hiding this comment.
아하 빌더 어노테이션이 있었다면 없을 로직인데, 빌더를 직접 구현하신걸 지금 확인했네요! ㅠㅠ 이해했습니다
| import kitchenpos.dao.TableGroupDao; | ||
| import kitchenpos.domain.TableGroup; | ||
|
|
||
| public class FakeTableGroupDao implements TableGroupDao { |
There was a problem hiding this comment.
Fake vs Mock
아주 ~ 예전에 코드를 작성할 때 테스트 더블이라는 키워드를 발견하고 실습했던 경험이 있습니다!
그때 Fake 객체는 Dao 에 아주 의존적이기 때문에 DAO 가 바뀌면 무조건 Fake 도 바꿔줘야해서 상당히 힘들었어요.
mock 도 마찬가지지만, 파라미터 값은 any() 나 다른 방법들을 사용해서 변경되더라도 기존 테스트 코드에는 지장없게 하는 방법도 있고, 실제 dao 로직과는 무관하게 동작하기 때문에 오히려 유연한 구조가 아닐까? 라고 생각 정리를 했었어요.
이런 관점도 있다! 말씀드리고 싶었습니다.
There was a problem hiding this comment.
안그래도 수업을 듣고 가짜 객체로 만들지, mocking 할지 고민이 많았는데요..
1단계에서는 추가하지 않았지만 이후에 Controller 테스트를 추가한다면 mocking 하는 방법을 선택할듯합니다. Controller 에서는 보통 하나의 Service 메서드를 호출하니까요.(서비스 메서드가 변경되더라도 영향을 받는 테스트가 적다는 의미입니당) 또한 서비스 계층의 메서드가 변경되더라도 일반적으로 내부 및 DTO 의 값이 변경될 뿐이지 서비스 메서드의 스펙 자체가 변동될 일은 적다고 생각합니다.
그런데 Service 테스트의 경우 현재 프로젝트 구조에 따르면 DAO 를 사용합니다. DAO 는 DB 에 저장되어 있는 데이터를 관리하는 계층이기 때문에 테스트가 복잡해짐에 따라 이전의 환경을 셋팅해야 하는 경우가 많더라구요.(Menu 를 만들기 위해서 MenuGroup 이 필요하는 등) 그럴때마다 하나의 테스트에 mocking 관련 로직만 무수히 많아져 테스트의 목적을 파악하기 어려웠어요. 그래서 실제로 호출하는 메서드와 유사한 로직을 호출하여 테스트의 의미를 표출하되, 실제 DB 와는 독립적으로 수행하기 위해 가짜 객체를 만들었습니다! 😁
좋은 내용 공유 감사합니다~ 덕분에 댓글로 작성하면서 다시 정리했어요 😊😊😊
안녕하세요 수달!
처음 미션에서 만났는데 마지막 미션에서 또 만날 수 있어 너무 좋네요 😁
처음과 마지막을 수달과 함께 할 수 있어서 좋아요
그런데 그런 마음과는 달리 미션을 너무 늦게 제출해서 죄송합니다..ㅠ.ㅠ
제출 시간을 지나서 제출하는데도 불구하고 코드의 상태가 좋지 않아 죄송할 뿐입니다..😭😭😭
아직 미션의 기간이 남았으니 계속해서 리팩토링을 진행하겠습니다! (물론 리뷰 전 코드가 변경된다면 따로 말씀드릴게요)
미션이 레거시 코드를 리팩토링하는 과정이니만큼 코드를 변경하고 싶은 마음이 드는 부분이 많았는데요,
이후에 진행될 단계에서 천천히 추가하도록 하겠습니다.
우테코 마지막 미션 리뷰 잘 부탁드립니다!! 🙇🏻♀️