리팩토링 계획

오랜만에 블로그에 글을 쓴다. 

지난 며칠 동안 프로젝트를 배포하기 위해 AWS 망구성을 하고, 프로젝트를 빌드하며 배포까지 시켰다. 
부하 테스트를 앞두고, 좋은 기회를 얻어 코드 리뷰를 받게 되었다. 

결론은, 이 상태로 부하 테스트를 진행하는 의미가 없을 정도로 심각한 문제가 많았다.

프로젝트를 시작하며 아쉬운 코드 작성 부분도 많았고, 궁금증도 많았지만, 나머지 주관적인 판단하에 깔끔한 부분은 나름 괜찮게 작성을 했다고 생각했었다. 

아직 고칠게 많았고, 배울게 많았고, 몰랐던 것이 많은 나였다. 이런 리뷰를 들을 때마다 나는 아직 한참 멀었구나. 

공부 기간 대비 많은 성장을 거두었다고 생각했는데, 내가 욕심이 많은 건지. 
사실은 기간 대비 성장치가 평균치였던 건지.

무기 해진다.

너무 급한가?
그럼에도 빨리 취업해야 하는 이유가 있다. 날 더 쪼아야 될 것이다. 어쩔 수 없다.

좋은 기회가 생겼다. 고민하느라 머리가 깨질 것 같다. 
더 좋은 부분을 이성적으로 추려 비교하는 과제가 스스로에게 주어졌다. 

Module 관련

모듈 이름이 module-common으로 지은 이유

 

> 해당 모듈은 모놀리식으로 되어있는데, 보통 다른 모듈에서 공통적으로 사용하는 모듈의 이름이 common이라 한다. 사용처에 맞게 이름을 고민해봐야 될 것 같다.

 

> 모듈을 나눈 이유는 MSA 환경을 고려해 본 것이 제일 크다. 

본 서버에 장애가 있을 때 이메일 전송 기능은 장애 격리를 시키게 위해 별개의 애플리케이션으로 동작할 수 있게 멀티 모듈을 고안하게 되었다.

 

> 결론적으로 기능 별로 모듈화를 해야 할지 크게 고민이 된다. 

공통, api, mail??

 

객체 매핑 시 model mapper를 택한 이유

> dto와 VO, dto와 Entity 매핑을 할 목적에만 집착하고, 성능을 고려하지 않았던 것 같다.

무작정 사용하는 것이 아닌 성능을 집착하는 습관을 들여야겠다.

 

> MapStruct를 사용해서 고쳐보자.

 

컨트롤러의 책임에 관하여

> 컨트롤러는 request, response의 역할이 주된 목적이다. 어떤 값을 알고 있을 필요가 없다. 

 

> enum으로 관리하거나, Service에서 알고 있으면 좋을 듯싶다.

 

스칼라와 응답 메시지

> Path로 받는 스칼라 값은 위험하다. 값이 바뀌게 되면 불필요한 수정이 이뤄질 것이다. 

이건 이미 알고 있었는데도, 놓친 부분이다. 무조건 매핑 매핑 매핑.

 

> 응답 코드가 200일 경우는 굳이 ResponseEntity 객체를 사용할 이유가 없을 것이다. 

물론 본인의 코드 작성 규칙을 정립한 경우 그에 합당한 이유가 있으면 좋다.

 

 

상태를 갖고 있는 Bean

> Bean은 가변적인 상태를 갖고 있으면 위험하다. Thread-Safe 하지가 않기 때문이다.

기본적인 개념인데, 좀 더 신중할 필요가 있다.

 

> 이게 부하 테스트를 중단한 가장 큰 이유이다. 거국적인 수정이 필요하겠다.

WAS는 항상 멀티스레드 기반으로 동작하는 것을 잊지 말자.

 

Null 체크와 Exception 처리

> 컨트롤러는 이러한 로직을 가질 이유가 없다. 최대한 서비스에서 처리

> 이럴 경우 Exception을 던져, RestControllerAdvice에서 공통 처리

> Null 체크 관련은 Optional을 적용해보자.

 

코드의 추상화 수준을 고려

> 추상화 수준은 항상 일관되어 있어야 가독성이 좋다. 

 

주문, 결제 도메인 관련

> 의존성을 확실히 분리하자

 

Optional 

> null을 반환하지 말자.

> 차라리 비어있는 쿠키를 반환

 

Map의 사용

> Util 클래스가 아닌 경우 Map 반환을 최대한 지양해야 한다. 타입 추론이 어려워진다.

> 일반 클래스를 만들어주자.

> Map을 이용할 경우 내부에서 처리할 때만.

 

Service Layer 함수의 다른 패키지 타입의 return

> Cookie는 javax.servler.http 패키지의 클래스. 이 날것을 그대로 리턴할 경우, 변경 시에 정말 취약해진다

> 패키지의 의존성을 생각하자.

> 이 역시 매핑

 

이벤트 퍼블리셔 관련

> 이벤트 퍼블리셔는 보통 비동기 처리 시 활용.

> 해당 로직은 절차적인 로직이므로, 비동기와 관련이 없다. 따로 메서드로 대체해도 될 것 같다.

 

RestTemplate과 try-catch

> restTemplate은 deprecated 되었으므로, webClient 사용을 고민

> catch라는 것은 에러를 해결했다는 의미인데, 단순히 log로 끝내는 건 황당한 경우이다. Exception을 던져서 해결하는 습관을 기르자.

> null 리턴은 항상 금물 => 밖에서 null 체크를 하게 되는 불상사가 발생

 

 

생각해보니, 저장소를 하나 더 파는게 좋을 수도 있을 것 같다.