[MVC 구현하기 - 2, 3단계] 다니(이다은) 미션 제출합니다. #85
Conversation
| database.put(recreateUser.getAccount(), recreateUser); | ||
| } | ||
|
|
||
| private static User recreateWithId(User user) { |
There was a problem hiding this comment.
리플렉션으로 User id를 바꿔주셨군요 👍👍👍
개인적인 의견일 수 있는데요.
recreateWithId 라는 이름과 User 를 반환하는 것을 봤을 때, 새로운 User가 생성되는 것처럼 느껴져요.
다른 흐름으로 만들어봐도 좋을 것 같습니다 !
There was a problem hiding this comment.
이 커멘트의 의도는 “메서드의 반환 타입을 void 형으로 만드는 게 메서드 내용과 어울리지 않을까?” 라는 생각이었습니다😉 이전에 말했던 것처럼 return 을 한다는 건 다른 참조가 될 거라고 오해할 수도 있다고 생각했어요.
| @@ -17,7 +17,7 @@ dependencies { | |||
|
|
|||
| implementation 'org.reflections:reflections:0.9.11' | |||
|
|
|||
| implementation 'com.fasterxml.jackson.core:jackson-databind:2.12.4' | |||
| implementation 'com.fasterxml.jackson.core:jackson-databind:2.12.5' | |||
| @@ -21,68 +16,72 @@ | |||
|
|
|||
| private final Object[] basePackages; | |||
| private final Map<HandlerKey, HandlerExecution> handlerExecutions; | |||
| private final HandlerScanner handlerScanner; | |||
There was a problem hiding this comment.
HandlerScanner 를 필드로 둔 기준이 궁금합니다 🤔
현재 흐름에서는 initialize() 에서 객체를 생성해서 사용해도 괜찮을 것 같아서 질문드려요!
미래를 위한 설계일까요?? 자유롭게 의견주세요!
There was a problem hiding this comment.
음.. 코멘트를 읽고 생각해봤는데, AnnotationHandlerMapping이 HandlerScanner를 가지지 않는 게 더 자연스러울 것 같네요!
미래를 위한 설계는 아니었어요! 리팩토링 하겠습니다 ~!
There was a problem hiding this comment.
중간곰은 클래스 내의 필드를 어떤 기준으로 두시나요? 궁금해요 !!
There was a problem hiding this comment.
이 커멘트를 남긴 이유는 제가 미션 할 때 필드로 둘 지, 안 둘지 고민했었고 저는 필드로 두지 않았기 때문에 다니의 생각이 궁금했어요!🤔
현재 미션에서 저는 필드로 두지는 않았지만 , 필드로 두고 외부에서 주입하는 게 유연한 코드라고 생각합니다. 인터페이스화 되어있다면 다양한 구현체를 쓸 수도 있겠고, 테스트도 용이하기도 하고, 같은 객체를 다른 객체에 주입해서 사용 할 수도 있고요.
하지만 현재 AnnotationHandlerMapping을 초기화하는 흐름을 봤을 때는 HandlerScanner는 AnnotationHandlerMapping에 완전 종속되어 있고, 기존 다니 코드의 주 생성자가 private 이기도 했고, initialize() 에서 한 번쓰이고 있었기 때문에 필드로 두지 않아도 되겠다는 생각이었어요. 물론 이 부분은 나중에 프로그램이 커지면서 언제든지 바뀔 수 있다고 생각합니다.
| this.handlers = handlers; | ||
| } | ||
|
|
||
| public void scan(Object[] basePackages) { |
There was a problem hiding this comment.
클래스의 사용법이 좀 헷갈리는 것 같아요.
- 객체를 생성 하고 -> scan() 을 호출하여 handlers 필드를 채우고
- getHandlers() 를 호출하여 채워진 handlers 를 가져오는 흐름이라고 이해했습니다.
의도하신 것 일수도 있지만 scan() 을 반복해서 실행하면 handlers 필드도 계속 누적되는 흐름이네요.
의도 하신 게 아니라면 clear() 해야 할 것 같기도 하고요🤯
주관적일 가능성이 큰데 Scan 이라는 이름이 클래스의 동작과 어울리지 않다는 생각이 들어요.
scan 메서드가 handlers 를 반환하는 게 어떤가요? 의견이 궁금합니다 ~
There was a problem hiding this comment.
클래스 사용법은 정확히 저게 맞아요!
중간곰 코멘트에서 궁금한 부분이 있는데요.
의도하신 것 일수도 있지만 scan() 을 반복해서 실행하면 handlers 필드도 계속 누적되는 흐름이네요.
의도 하신 게 아니라면 clear() 해야 할 것 같기도 하고요🤯
handlers는 map이기 때문에 scan()을 반복해서 실행해도 handlers 필드가 계속 누적되는 건 아니지 않나요?!
There was a problem hiding this comment.
모든 Handler(Controller)를 스캔해서 초기화한다는 의미로 scan()으로 네이밍을 했어요!
초기화 의미가 부족한 것 같다면, scan() -> initialize()로 이름을 바꾸는 게 좋을까요?
There was a problem hiding this comment.
다른 basepackages 로 scan()을 여러 번 호출하는 상황을 말한 거였습니다.
지금처럼 handler 를 저장하는 흐름이면 store() 같은 느낌을 줘야하지 않을까 ?? 라는 생각이었습니다.🧐
|
중간곰 ~~! 🐻 피드백 바탕으로 리팩토링 완료하고, 테스트도 추가했습니다! 바쁜 와중에 코드 리뷰 해줘서 감사합니다 🙇♀️ |
|
SonarCloud Quality Gate failed. |
| @Override | ||
| public void render( | ||
| Map<String, ?> model, | ||
| HttpServletRequest request, | ||
| HttpServletResponse response | ||
| ) { | ||
| response.setContentType(APPLICATION_JSON_UTF8_VALUE); | ||
|
|
||
| if (model.size() == 1) { |
There was a problem hiding this comment.
지금 보니까 JsonConverter 가 있으니 그 쪽으로 위임하는 것 도 좋겠네요 🤗
안녕하세요, 중간곰 ~! 🐻
1단계 코드 리뷰 감사합니다 ㅎㅎㅎ 피드백 반영했어요!
2, 3단계도 요구사항에 맞춰 구현 완료했습니다!
아직 asis 패키지에 있는 클래스들과 ManualXXXController 클래스들은 삭제하지 않았어요.
틈틈이 테스트 코드 작성해가면서 지울 생각입니다!
이번에도 좋은 코멘트 많이 남겨주세요 ~!
잘 부탁드립니다! 🙇♀️🙌