[2단계 - 점진적인 리팩터링] 노랑 미션 제출합니다#986
Conversation
- ControllerScanner 추가 - 개행 맞춤 - 필드 이름 변경
- `DispatcherServlet`에서 핸들러 매핑 처리 로직 수정 - 새로운 인터페이스 `HandlerMapping` 정의 및 구현
- 어노테이션 기반 컨트롤러를 통해 폼 페이지(`/form`)와 결과 페이지(`/result`) 처리 기능 구현 - `Step2Controller` 추가하여 GET 및 POST 요청 대응 - `form.jsp` 및 `result.jsp` 뷰 파일 생성하여 사용자 입력 및 결과 표시
- `HandlerAdapter` 인터페이스 및 구현체 추가 (`ManualHandlerAdapter`, `AnnotationHandlerAdapter`) - 기존 핸들러 매핑 및 레지스트리를 리팩터링하여 어댑터 패턴 적용 - `DispatcherServlet`에서 핸들러와 어댑터를 활용해 요청 처리 로직 개선 - 불필요한 메서드 제거 및 코드 간소화
- 필요 없는 테스트 메서드 코드 제거
|
@coderabbitai review |
|
@yesjuhee 네, 지금 PR을 리뷰하겠습니다! 말씀하신대로 테스트용 파일들( HandlerMapping, HandlerAdapter 패턴을 통한 확장 가능한 구조와 레거시 Controller 인터페이스와 @controller 어노테이션 기반 MVC의 공존 구조를 중심으로 살펴보겠습니다. ✅ Actions performedReview triggered.
|
WalkthroughDispatcherServlet이 핸들러 매핑/어댑터 레지스트리 기반으로 재구성되어 HandlerMappingRegistry와 HandlerAdapterRegistry에서 핸들러·어댑터를 조회하고 어댑터가 반환한 ModelAndView를 private render()로 처리하도록 변경됨. public init()은 제거됨. 새 인터페이스 HandlerMapping/HandlerAdapter 및 구현들(Manual/Annotation 매핑·어댑터), HandlerMappingRegistry, HandlerAdapterRegistry, ControllerScanner가 추가되고 HandlerExecution/HandlerKey의 패키지 이동과 NoHandlerFoundException이 도입됨. 또한 Step2Controller와 form.jsp·result.jsp 뷰가 추가됨. Pre-merge checks❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (48)
mvc/src/main/java/com/interface21/webmvc/servlet/View.java (4)
9-9: 예외 타입을 구체화해 View 계약 명확화
throws Exception은 너무 포괄적입니다. 렌더링 실패의 성격(입출력/뷰 해석/템플릿 처리)에 따라 예외를 구분하면 호출 측(DispatcherServlet)의 처리 전략이 선명해집니다.
- 만약 I/O 실패와 뷰 해석 오류가 다르다면, 어떻게 구분해 전파하시겠나요?
- 예외 메시지/상태코드 매핑 정책은 어디에 둘 계획인가요(뷰 vs 디스패처)?
9-9: model을 일급 컬렉션/도메인 객체로 포장 고려 (규칙 3, 8)
Map<String, ?>은 사용이 편하지만 도메인 개념이 흐려지고 유효성 보장이 어렵습니다.
- 특정 키 스키마가 있다면 전용
Model/ViewModel객체로 포장하면 응집도와 검증이 좋아집니다.- 포장을 통해 필수 키 누락, 타입 불일치를 컴파일타임/생성 시점에 막을 수 있습니다.
왜: 원시값/문자열 포장과 일급 컬렉션 원칙은 도메인 의미를 코드에 드러내고 테스트 용이성을 높입니다.
9-9: Null/불변성 계약을 명시하고 수용성 결정
model이 null일 수 있는지, 변경 가능한지(뷰가 put/remove 해도 되는지)가 모호합니다.
- null 금지라면 호출부에 빈 모델 보장을 요구하시겠나요?
- 읽기 전용이라면 불변 뷰(예: 수정 시 예외)로 전달하는 게 맞을까요?
- 변경 허용이 필요하면 변경 지점과 스레드 안정성은 어떻게 보장할까요?
7-10: JavaDoc로 렌더링 책임 경계 문서화계약 수준에서 다음을 명시하면 학습 비용과 결합도가 줄어듭니다.
- contentType/charset 설정 책임(뷰 vs 디스패처)
- 상태코드/리다이렉트 처리 방식
- 멱등성 보장 여부, 다중 렌더링 방지 규칙
- 사용 가능한 표준 모델 키 목록(예: viewName, errors 등)
mvc/src/main/java/com/interface21/webmvc/servlet/mvc/asis/Controller.java (1)
7-7: 불필요한 빈 줄이 추가되었습니다인터페이스 내부에 빈 줄을 추가하는 것은 일반적인 Java 코드 스타일과 맞지 않습니다. 인터페이스 선언과 메서드 선언 사이에는 빈 줄이 필요하지 않으며, 코드의 일관성을 위해 제거하는 것이 좋겠습니다.
mvc/src/main/java/com/interface21/web/WebApplicationInitializer.java (1)
7-7: 불필요한 빈 줄이 추가되었습니다Controller 인터페이스와 마찬가지로 여기서도 인터페이스 내부에 불필요한 빈 줄이 추가되었습니다. 코드 스타일의 일관성을 위해 제거를 고려해보세요.
mvc/src/main/java/com/interface21/webmvc/servlet/mvc/tobe/handler/mapping/ControllerScanner.java (3)
13-21: 클래스 설계는 적절하나, 객체지향 원칙을 더 고려해보세요ControllerScanner 클래스가 컨트롤러 스캔과 인스턴스 생성이라는 명확한 책임을 가지고 있어 좋습니다. 다만 몇 가지 개선점을 고려해보시면 어떨까요?
- Reflections 객체를 직접 인스턴스 변수로 가지는 것보다, 스캔 기능을 인터페이스로 추상화하면 테스트가 더 용이해질 것 같은데요?
- 생성자에서 Object... 타입보다는 String... 또는 Package[] 같은 더 명확한 타입을 사용하면 의도가 더 명확해질 것 같습니다.
38-48: 예외 처리와 관련하여 몇 가지 고려사항이 있습니다현재 구현에서 몇 가지 개선할 수 있는 부분이 있어 보입니다:
로깅 언어 일관성: 한글 에러 메시지("컨트롤러 기본 생성자가 필요합니다")가 있는데, 프로젝트 전체에서 로그 메시지 언어를 통일하는 것이 어떨까요?
예외 처리 전략: 모든 예외를 RuntimeException으로 감싸서 던지고 있는데, 더 구체적인 커스텀 예외를 만들어 사용하면 어떨까요? 예를 들어
ControllerInstantiationException같은 도메인 특화 예외를 만들면 호출하는 쪽에서 더 명확하게 처리할 수 있을 것 같습니다.리플렉션 접근 권한:
getDeclaredConstructor()를 사용하고 있는데, private 생성자인 경우setAccessible(true)가 필요할 수 있습니다. 아니면 public 생성자만 지원한다는 제약을 명시하는 것도 방법일 것 같은데요?이러한 점들을 고려해서 예외 처리를 개선해보시면 어떨까요?
23-26: 메서드 명명과 반환 타입에 대해 생각해보세요
getControllers()메서드가Map<Class<?>, Object>를 반환하는데, 이 부분에 대해 몇 가지 제안이 있습니다:
메서드 이름이 단순히 컨트롤러들을 가져온다는 의미인데, 실제로는 클래스와 인스턴스의 매핑을 반환합니다.
getControllerInstances()같은 더 명확한 이름은 어떨까요?반환 타입의 Value가 Object인데, 이를 더 구체적인 타입으로 제한할 수 있을까요? 예를 들어 모든 컨트롤러가 구현해야 하는 마커 인터페이스나 베이스 클래스가 있다면 그것을 사용하는 것이 타입 안정성에 도움이 될 것 같습니다.
mvc/src/main/java/com/interface21/webmvc/servlet/mvc/tobe/handler/mapping/HandlerExecution.java (4)
18-20: 반환 타입 계약 불명확 — ModelAndView 강제/검증 필요현재 반환값을 ModelAndView로 단언(cast)하고 있어, 핸들러가 다른 타입을 반환하면 ClassCastException 위험이 있습니다. 등록 시 반환 타입을 검증하거나, HandlerAdapter에서 다양한 반환 타입을 어댑팅하도록 역할을 분리하는 방향을 고려해보셨나요?
18-20: 예외 매핑 일관성InvocationTargetException 등 반사 호출 예외가 프레임워크 전용 예외로 일관되게 매핑되지 않습니다. 상위 레이어(Dispatcher/Adapter)가 예외 처리를 표준화할 수 있도록 공통 예외로 변환하는 정책을 정해보면 어떨까요?
13-16: 널/불변성 사전조건 명시declaredObject와 method에 대한 null 불가 전제와 불변(스레드 간 공유) 가정이 코드에서 암묵적입니다. 사전조건을 문서화(또는 방어)하고, 컨트롤러 인스턴스가 상태를 갖는 경우의 스레드‑세이프티를 어떻게 보장할지 고민해보셨나요?
10-10: 이름 의도 명확성declaredObject보다는 역할을 드러내는 이름(예: controller/handler)이 가독성에 유리합니다. 팀 합의된 네이밍 가이드를 확인해보셨나요?
mvc/src/main/java/com/interface21/webmvc/servlet/mvc/tobe/HandlerMapping.java (1)
5-8: 계약(Contract) 명시 강화 필요getHandler가 Object를 반환하므로 호출자는 런타임 캐스팅에 의존합니다.
- 어떤 타입들을 반환 가능한지(예: Controller, HandlerExecution 등)와 “찾지 못한 경우 null 반환 vs 예외” 정책을 Javadoc/애노테이션(@Nullable/@nonnull)으로 명시해 주실 수 있을까요?
- Registry가 예외를 던지는 정책이라면, 구현체는 null을 반환한다는 합의를 문서화하면 혼동을 줄일 수 있습니다.
mvc/src/test/java/com/interface21/webmvc/servlet/mvc/tobe/AnnotationHandlerMappingTest.java (4)
33-37: 핸들러 호출은 Adapter가 담당하도록 테스트 관심사 분리Mapping 단위 테스트에서 HandlerExecution.handle까지 호출하면 매핑/호출 책임이 섞입니다.
- 이 테스트는 “핸들러를 올바르게 찾는다”에 집중하고, 실제 호출은 HandlerAdapter(또는 통합 테스트)로 위임해보면 어떨까요?
48-52: GET/POST 중복 패턴 통합GET/POST 케이스가 구조적으로 동일합니다. 파라미터라이즈드 테스트 또는 헬퍼 메서드로 중복을 제거해 가독성과 유지보수성을 높여보는 건 어떨까요?
18-22: 매직 스트링(base package) 의미 드러내기"samples" 문자열은 테스트 의도를 숨깁니다. 상수화하거나 테스트 픽스처 이름에 의도를 드러내면 추후 패키지 변경에도 안정적입니다.
24-37: 부정 경로(미등록 URL/중복 매핑 충돌)도 검증해보세요미등록 URL 시 동작(Null/예외)과, 중복 매핑 시 예외(설계상 IllegalArgumentException)를 검증하면 회귀를 방지할 수 있습니다. 어떤 동작이 “정답”인지 팀 합의를 먼저 명문화해 보실래요?
Also applies to: 39-52
mvc/src/main/java/com/interface21/webmvc/servlet/mvc/tobe/handler/adapter/HandlerAdapterRegistry.java (4)
12-17: 어댑터 등록 순서가 우선순위를 암묵적으로 결정합니다. 의도된가요?Manual → Annotation 순서가 충돌 시 해석 우선권을 가집니다. 이 우선순위가 요구사항인지 확인하고, 변경 가능성이 있다면 구성(생성자 주입, 설정 기반)으로 노출하는 방안을 고려해보세요.
19-26: 예외 메시지에 디버깅 단서를 추가해주세요.현재 "No handler adapter"만으로는 원인 파악이 어렵습니다. 실패한 handler의 실제 타입/클래스명을 포함하면 운영 가시성이 좋아집니다. 왜? 장애 상황에서 해당 타입을 지원하는 어댑터 추가 여부를 즉시 판단할 수 있습니다.
10-17: 불변성 강화로 읽기 전용 레지스트리를 보장해보면 어떨까요?생성자 이후에는 목록이 변하지 않습니다. 컬렉션을 불변화(외부 노출 차단)하면 다중 스레드 환경에서 안전성이 높아집니다. 왜? 우발적 추가/삭제에 의한 디스패치 일관성 저하를 예방합니다.
20-25: 디미터 법칙(한 줄에 점 하나) 관점에서 스트림 체인을 단순화할 여지가 있습니다.과도한 체인은 디버깅과 리딩 비용을 높일 수 있습니다. 필터링/선정 로직을 메서드로 추출해 의도를 드러내는 것도 방법입니다.
mvc/src/main/java/com/interface21/webmvc/servlet/mvc/tobe/HandlerAdapter.java (2)
7-13: 핸들러 타입 안전성과 계약(Contract) 명시가 필요합니다.
- Object 기반 시그니처는 유연하지만 타입 안전성이 약합니다. 제네릭 도입 또는 핸들러 마커 인터페이스로 범위를 좁히는 대안을 고려해보셨나요?
- handle가 어떤 부작용(예: response commit)을 허용/금지하는지 자바독으로 계약을 명확히 하면 구현/테스트 품질이 좋아집니다. 왜? 어댑터 간 일관된 동작을 보장합니다.
11-12: 너무 넓은 예외 선언은 호출자 처리 전략을 흐립니다.throws Exception 대신 도메인별 예외(또는 래핑)로 좁히면 상위 레이어에서의 책임 분리가 선명해집니다.
app/src/main/java/com/techcourse/HandlerMappingRegistry.java (3)
37-39: 메서드명 오탈자(adHandlerMapping)로 가독성이 떨어집니다.일관된 명명은 의도를 전달하는 기본 전제입니다. 오탈자 수정으로 유지보수 비용을 줄이세요.
15-25: 매핑 등록/우선순위가 고정되어 있습니다. 의도된 불변인가요?
- Manual → Annotation 순서가 충돌 시 라우팅 우선권을 결정합니다. 이 정책이 요구사항이라면 테스트로 명시해보세요.
- 확장(새 HandlerMapping 추가) 필요 시 구성 주입으로 열어둘 여지도 고민해보셨나요?
28-35: 디미터 법칙과 가독성 측면에서 스트림 체인을 단순화할 수 있습니다.해결 전략을 캡슐화한 메서드로 추출하면 의도가 더 분명해집니다. 왜? “첫 번째로 처리 가능한 매핑 조회”라는 유즈케이스를 도메인 용어로 드러낼 수 있습니다.
mvc/src/main/java/com/interface21/webmvc/servlet/mvc/tobe/handler/adapter/ManualHandlerAdapter.java (1)
23-24: 컨트롤러가 null/빈 문자열을 반환할 때의 방어 로직을 고려해보셨나요?뷰 이름이 유효하지 않으면 포워딩/리다이렉트 오류로 이어집니다. 계약상 허용하지 않는다면 명시적 검증과 실패 전략(예외, 기본 뷰)을 정의해보세요.
app/src/main/java/com/techcourse/ManualHandlerMapping.java (5)
23-34: initialize 메서드가 하는 일이 많습니다(규칙 6: 메서드 10줄 이하).매핑 등록, 로깅을 분리해 단일 책임을 강화해보면 어떨까요? 왜? 테스트 용이성↑, 변경 영향 범위↓.
21-34: 정적 가변 Map 사용은 재초기화/테스트 간 간섭 위험이 있습니다.초기화 재호출 시 중복/덮어쓰기, 테스트 간 상태 누수 가능성을 점검해보세요. 불변화 또는 생애주기 관리(초기화 한정)를 고려해보면 좋습니다.
24-31: 원시값(String 경로) 남용(규칙 3)으로 의도 표현력이 제한됩니다.경로/라우트 개념을 값 객체로 포장하면 검증(중복/형식)과 정책(리다이렉트 여부 등)을 응집시킬 수 있습니다. 만약 그렇게 한다면, 어떤 부수효과(테스트, 매핑 확장)가 기대될까요?
32-34: 디미터 법칙(규칙 4): 체이닝 로깅을 의도 중심 API로 감싸보면 어떨까요?컨트롤러 목록 출력 책임을 메서드로 위임하면 호출부는 “매핑 현황 로깅”만 표현하게 되어 가독성이 올라갑니다.
36-41: HandlerMapping 구현 반환형 차이를 팀 규약으로 명시해 주세요.현재 Controller/HandlerExecution 등 서로 다른 타입을 반환합니다(Object 수용). 어댑터 레이어로 정규화되지만, 구현 가이드(무엇을 반환할지)를 문서화하면 혼선을 줄일 수 있습니다.
mvc/src/main/java/com/interface21/webmvc/servlet/mvc/tobe/handler/mapping/AnnotationHandlerMapping.java (7)
28-36: initialize 호출 시점과 스레드 안전성 확인레지스트리에서 이 매핑의 initialize()가 반드시 서비스 전 한 번만 호출된다는 계약이 보장되나요? 멀티스레드 초기화 경쟁이나 지연 초기화 시 NPE/미등록 상태가 생기지 않도록 라이프사이클을 명확히 해보세요.
46-50: 비공개 메서드 스캔 범위 재검토getDeclaredMethods()는 private/protected도 포함합니다. HandlerExecution이 접근 제어 이슈를 안전하게 처리하나요? 공개 메서드만 허용할지, 혹은 접근 제어를 명시적으로 관리할지 정책을 정리해보세요.
60-73: URL 정규화 일관성 확보@RequestMapping의 value(슬래시 중복, 트레일링 슬래시, 파라미터/세미콜론 매트릭스 변수 등)를 그대로 키로 쓰면 미묘한 불일치로 중복 또는 누락 매핑이 생길 수 있어요. 경로 정규화 규칙을 한 곳으로 통합하는 방식을 고려해보셨나요?
75-87: 중복 등록 예외 타입/메시지 개선현재 warn 로그 후 IllegalArgumentException을 던지는데, 어느 키/컨트롤러 충돌인지 진단 가능하도록 예외 타입/메시지를 구체화해보세요. 운영 시 원인 파악 속도에 큰 차이가 납니다.
46-49: 객체지향 생활 체조 규칙 4(한 줄에 점 하나) 고려stream/filter/map/toList 체이닝이 다수 등장합니다. “한 줄에 점 하나” 원칙을 지키도록 중간 연산을 명명된 단계로 분리하거나, 의도를 드러내는 헬퍼로 추출해보면 가독성이 올라가요. 어떤 이름이 가장 의도를 잘 드러날까요?
Also applies to: 60-69, 70-73
16-26: 클래스 책임 슬림화스캔/키 생성/등록/조회가 한 클래스 안에 공존합니다(규칙 6: 엔티티를 작게). 역할을 분리하면 테스트 용이성과 확장성이 좋아집니다. 예: 스캐너, 키 팩토리, 레지스트리 분리. 어떤 경계가 자연스러울지 그림을 그려보실래요?
Also applies to: 28-36, 52-58, 75-87
20-26: basePackage 타입 안정성 확인Object... varargs는 오입력 가능성이 큽니다. 허용 타입(String/Package 등)을 제한하거나 검증 로직을 두는 게 안전합니다. ControllerScanner의 생성자 계약과 일치하는지 점검해보셨나요?
app/src/main/java/com/techcourse/DispatcherServlet.java (8)
17-23: 필드 네이밍 일관성handlermappingRegistry는 카멜 케이스가 깨져 보여요(handlerMappingRegistry 권장). 작은 네이밍 균일성이 유지보수 가독성에 기여합니다.
26-39: 예외 처리: 원인 손실 및 과도한 캐치 범위Throwable을 포괄적으로 캐치하고 새 ServletException(message)로 감싸면 cause 체인이 유실됩니다. Error까지 잡을 필요가 있는지도 고민해보세요. 원인 보존과 상태 코드 매핑 전략(404/405/500)을 명확히 해볼까요?
31-34: 핸들러 미발견/어댑터 미발견 시 HTTP语义 점검미발견은 404, 메서드 부적합은 405로 매핑되는지요? 현재 흐름상 예외가 ServletException으로 치환되어 500이 될 위험이 있습니다. 레지스트리/어댑터 레이어에서 의미별 예외를 정의하고 매핑하는 방식을 고려해보세요.
41-46: 디미터 법칙 및 Tell, Don’t Askmav.getView().render(...)는 체인 접근이 발생합니다(규칙 4 위반). 렌더링 오케스트레이션을 ModelAndView(혹은 ViewResolver)로 위임하는 구조를 고민해보셨나요? 호출자는 “렌더해라”만 말하면 됩니다.
26-39: 메서드 길이 축소(규칙 6: 메서드 10줄 이하)service 메서드가 다소 길고 여러 책임(해석-적응-실행-렌더)을 담고 있어요. 각 단계를 작은 메서드로 분리하면 예외 매핑도 자연스럽게 모듈화됩니다. 어떤 경계로 쪼개면 테스트가 쉬워질까요?
41-46: throws Exception 범위 축소render가 광범위한 Exception을 던지면 상위에서 의미 있는 처리/로깅이 어려워집니다. 구체 타입으로 좁히거나 내부에서 처리하는 방향을 검토해보세요.
33-34: ModelAndView null 가능성 가드핸들러/어댑터가 null을 반환할 가능성(에러 흐름 등)을 배제하셨나요? NPE 방어 또는 계약 보장을 문서화/검증 테스트로 명확히 하는 것을 권장합니다.
20-23: 의존성 주입 가능성 열어두기레지스트리를 내부에서 new로 고정하면 테스트/확장 유연성이 떨어집니다. 생성자 주입(또는 팩토리/빌더)로 대체하면 대역 주입이 쉬워져요. 현재 단계에서 과하다고 느끼신다면 확장 포인트만 열어두는 것도 방법입니다.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (20)
app/src/main/java/com/techcourse/Application.java(1 hunks)app/src/main/java/com/techcourse/DispatcherServlet.java(1 hunks)app/src/main/java/com/techcourse/HandlerMappingRegistry.java(1 hunks)app/src/main/java/com/techcourse/ManualHandlerMapping.java(2 hunks)app/src/main/java/com/techcourse/controller/Step2Controller.java(1 hunks)app/src/main/webapp/form.jsp(1 hunks)app/src/main/webapp/result.jsp(1 hunks)mvc/src/main/java/com/interface21/web/WebApplicationInitializer.java(1 hunks)mvc/src/main/java/com/interface21/webmvc/servlet/View.java(1 hunks)mvc/src/main/java/com/interface21/webmvc/servlet/mvc/asis/Controller.java(1 hunks)mvc/src/main/java/com/interface21/webmvc/servlet/mvc/tobe/HandlerAdapter.java(1 hunks)mvc/src/main/java/com/interface21/webmvc/servlet/mvc/tobe/HandlerMapping.java(1 hunks)mvc/src/main/java/com/interface21/webmvc/servlet/mvc/tobe/handler/adapter/AnnotationHandlerAdapter.java(1 hunks)mvc/src/main/java/com/interface21/webmvc/servlet/mvc/tobe/handler/adapter/HandlerAdapterRegistry.java(1 hunks)mvc/src/main/java/com/interface21/webmvc/servlet/mvc/tobe/handler/adapter/ManualHandlerAdapter.java(1 hunks)mvc/src/main/java/com/interface21/webmvc/servlet/mvc/tobe/handler/mapping/AnnotationHandlerMapping.java(2 hunks)mvc/src/main/java/com/interface21/webmvc/servlet/mvc/tobe/handler/mapping/ControllerScanner.java(1 hunks)mvc/src/main/java/com/interface21/webmvc/servlet/mvc/tobe/handler/mapping/HandlerExecution.java(2 hunks)mvc/src/main/java/com/interface21/webmvc/servlet/mvc/tobe/handler/mapping/HandlerKey.java(2 hunks)mvc/src/test/java/com/interface21/webmvc/servlet/mvc/tobe/AnnotationHandlerMappingTest.java(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.java
⚙️ CodeRabbit configuration file
**/*.java: ## 🎯 코드 품질 중심 리뷰 가이드라인이 리뷰는 코드 품질과 객체지향 원칙에 집중합니다.
미션 달성 여부가 아닌, 코드 설계와 품질 개선에 대한 피드백을 한글로 제공해주세요.📚 학습 원칙
- 직접 코드를 제공하지 마세요 (학습자가 명시적으로 요청하는 경우 제외)
- 문제 해결 과정을 안내하되, 정답을 바로 알려주지 마세요
- 작은 단계로 문제를 분해하여 접근하도록 도와주세요
💡 피드백 방법
- 유도 질문 활용: "만약 ~라면 어떻게 될까요?", "~를 고려해보셨나요?"
- 힌트 제공: 방향은 제시하되, 구체적인 구현은 학습자가 하도록
- 다양한 접근법 제시: 한 가지 해결책이 아닌 여러 가능성을 제안
- 왜?에 집중: 단순히 무엇이 잘못되었는지보다 왜 그런지 이해하도록
⚡ 객체지향 생활 체조 원칙 검토
다음은 객체지향 생활 체조(Object Calisthenics) 9가지 원칙입니다.
위반 시 학습 효과를 위해 반드시 피드백을 제공하되, 왜 이 원칙이 중요한지 설명해주세요:규칙 1: 한 메서드에 오직 한 단계의 들여쓰기만
- 들여쓰기 depth 최대 1 (중첩 제어구조 금지)
- 📖 이유: 메서드 복잡도 감소, 단일 책임 원칙 강화
- 💡 힌트: "이 부분을 별도 메서드로 추출하면 어떨까요?"
규칙 2: else 예약어 금지
- else, switch/case 사용 금지
- 📖 이유: 복잡한 분기 제거, 명확한 코드 흐름
- 💡 힌트: "early return이나 가드 클로즈 패턴을 고려해보세요"
규칙 3: 모든 원시값과 문자열 포장
- 원시 타입과 String을 객체로 포장
- 📖 이유: 도메인 개념 명확화, 비즈니스 로직 응집
- 💡 예시:
int port→Port port,String name→Name name규칙 4: 한 줄에 점 하나만 (디미터 법칙)
- 메서드 체이닝 제한
- 📖 이유: 결합도 감소, 캡슐화 향상
- 💡 나쁜 예:
request.getUri().getPath().substring()- 💡 좋은 예:
request.extractPath()규칙 5: 축약 금지
- 명확한 이름 사용 (축약어 금지)
- 📖 이유: 코드 가독성, 의도 명확화
- 💡 예시:
req→request,calcAmt→calculateAmount규칙 6: 모든 엔티티를 작게 유지
- 클래스 50줄, 메서드 10줄 이하
- 📖 이유: 단일 책임, 이해와 테스트 용이성
- 💡 힌트: "이 클래스가 너무 많은 일을 하고 있지 않나요?"
규칙 7: 인스턴스 변수 3개 이하
- 클래스당 최대 3개의 인스턴스 변수
- 📖 이유: 높은 응집도, 단일 책임 유지
- 💡 힌트: "관련 필드들을 별도 객체로 묶을 수 있을까요?"
규칙 8: 일급 컬렉션 사용
- 컬렉션을 감싸는 클래스 사용
- 📖 이유: 컬렉션 로직 캡슐화, 불변성 보장
- 💡 예시:
List<HttpHeader>→HttpHeaders클래스규칙 9: 게터/세터/프로퍼티 금지
- Tell, Don't Ask 원칙 준수
- 📖 이유: 진정한 캡슐화, 객체 자율성
- 💡 나쁜 예:
if (user.getPassword().equals(input))- 💡 좋은 예:
if (user.authenticate(input))🎯 추가 프로그래밍 요구사항
코드 스타일
- Java Style Guide 준수
- 3항 연산자 사용 금지
테스트 및 구조
- **모든 기능 T...
Files:
mvc/src/main/java/com/interface21/webmvc/servlet/mvc/asis/Controller.javamvc/src/main/java/com/interface21/webmvc/servlet/mvc/tobe/HandlerMapping.javamvc/src/main/java/com/interface21/web/WebApplicationInitializer.javamvc/src/main/java/com/interface21/webmvc/servlet/mvc/tobe/HandlerAdapter.javamvc/src/main/java/com/interface21/webmvc/servlet/mvc/tobe/handler/adapter/AnnotationHandlerAdapter.javaapp/src/main/java/com/techcourse/HandlerMappingRegistry.javamvc/src/main/java/com/interface21/webmvc/servlet/mvc/tobe/handler/adapter/HandlerAdapterRegistry.javamvc/src/main/java/com/interface21/webmvc/servlet/mvc/tobe/handler/adapter/ManualHandlerAdapter.javaapp/src/main/java/com/techcourse/controller/Step2Controller.javamvc/src/main/java/com/interface21/webmvc/servlet/View.javamvc/src/test/java/com/interface21/webmvc/servlet/mvc/tobe/AnnotationHandlerMappingTest.javamvc/src/main/java/com/interface21/webmvc/servlet/mvc/tobe/handler/mapping/AnnotationHandlerMapping.javamvc/src/main/java/com/interface21/webmvc/servlet/mvc/tobe/handler/mapping/ControllerScanner.javaapp/src/main/java/com/techcourse/Application.javamvc/src/main/java/com/interface21/webmvc/servlet/mvc/tobe/handler/mapping/HandlerKey.javamvc/src/main/java/com/interface21/webmvc/servlet/mvc/tobe/handler/mapping/HandlerExecution.javaapp/src/main/java/com/techcourse/DispatcherServlet.javaapp/src/main/java/com/techcourse/ManualHandlerMapping.java
🧬 Code graph analysis (8)
mvc/src/main/java/com/interface21/webmvc/servlet/mvc/tobe/HandlerAdapter.java (1)
mvc/src/main/java/com/interface21/webmvc/servlet/ModelAndView.java (1)
ModelAndView(7-33)
mvc/src/main/java/com/interface21/webmvc/servlet/mvc/tobe/handler/adapter/AnnotationHandlerAdapter.java (2)
mvc/src/main/java/com/interface21/webmvc/servlet/ModelAndView.java (1)
ModelAndView(7-33)mvc/src/main/java/com/interface21/webmvc/servlet/mvc/tobe/handler/mapping/HandlerExecution.java (1)
HandlerExecution(8-21)
app/src/main/java/com/techcourse/HandlerMappingRegistry.java (1)
mvc/src/main/java/com/interface21/webmvc/servlet/mvc/tobe/handler/mapping/AnnotationHandlerMapping.java (1)
AnnotationHandlerMapping(16-88)
mvc/src/main/java/com/interface21/webmvc/servlet/mvc/tobe/handler/adapter/ManualHandlerAdapter.java (2)
mvc/src/main/java/com/interface21/webmvc/servlet/ModelAndView.java (1)
ModelAndView(7-33)mvc/src/main/java/com/interface21/webmvc/servlet/view/JspView.java (1)
JspView(10-38)
app/src/main/java/com/techcourse/controller/Step2Controller.java (2)
mvc/src/main/java/com/interface21/webmvc/servlet/ModelAndView.java (1)
ModelAndView(7-33)mvc/src/main/java/com/interface21/webmvc/servlet/view/JspView.java (1)
JspView(10-38)
mvc/src/test/java/com/interface21/webmvc/servlet/mvc/tobe/AnnotationHandlerMappingTest.java (2)
mvc/src/main/java/com/interface21/webmvc/servlet/mvc/tobe/handler/mapping/AnnotationHandlerMapping.java (1)
AnnotationHandlerMapping(16-88)mvc/src/main/java/com/interface21/webmvc/servlet/mvc/tobe/handler/mapping/HandlerExecution.java (1)
HandlerExecution(8-21)
app/src/main/java/com/techcourse/DispatcherServlet.java (2)
mvc/src/main/java/com/interface21/webmvc/servlet/ModelAndView.java (1)
ModelAndView(7-33)mvc/src/main/java/com/interface21/webmvc/servlet/mvc/tobe/handler/adapter/HandlerAdapterRegistry.java (1)
HandlerAdapterRegistry(8-31)
app/src/main/java/com/techcourse/ManualHandlerMapping.java (5)
app/src/main/java/com/techcourse/controller/LoginController.java (1)
LoginController(11-37)app/src/main/java/com/techcourse/controller/LoginViewController.java (1)
LoginViewController(9-22)app/src/main/java/com/techcourse/controller/LogoutController.java (1)
LogoutController(7-15)app/src/main/java/com/techcourse/controller/RegisterController.java (1)
RegisterController(9-21)app/src/main/java/com/techcourse/controller/RegisterViewController.java (1)
RegisterViewController(7-13)
🔇 Additional comments (11)
mvc/src/main/java/com/interface21/webmvc/servlet/View.java (1)
8-8: 형식 변경만: 동작 영향 없음공백 추가로 보이며, 동작·계약에는 영향 없습니다.
app/src/main/java/com/techcourse/Application.java (1)
3-6: import 순서가 변경되었네요Java 컨벤션에 따라 import를 정리하신 것은 좋습니다. 일반적으로 java/javax 패키지를 먼저, 그 다음 third-party 라이브러리를 배치하는 것이 표준이며, 현재 변경사항이 이를 잘 따르고 있습니다.
mvc/src/main/java/com/interface21/webmvc/servlet/mvc/tobe/handler/mapping/HandlerKey.java (2)
1-1: 패키지 이동이 적절합니다HandlerKey를 handler.mapping 패키지로 이동한 것은 좋은 선택입니다. 이 클래스가 핸들러 매핑과 직접적으로 관련되어 있으므로, 관련 클래스들과 같은 패키지에 위치하는 것이 응집도를 높입니다.
25-31: equals 메서드 가독성이 개선되었습니다단일 라인 조건문을 멀티라인 블록으로 변경한 것은 가독성 향상에 도움이 됩니다. 특히 디버깅 시 브레이크포인트를 설정하기 용이해집니다.
mvc/src/main/java/com/interface21/webmvc/servlet/mvc/tobe/handler/mapping/ControllerScanner.java (1)
28-36: 스트림 연산이 깔끔합니다함수형 프로그래밍 스타일로 컨트롤러 인스턴스화를 처리한 것이 좋습니다. toMap 컬렉터와 메서드 레퍼런스를 활용한 것도 적절합니다.
app/src/main/webapp/form.jsp (1)
1-64: PR 요청에 따라 리뷰 제외해당 JSP는 테스트용이며 리뷰 제외 요청을 반영해 코멘트를 생략합니다.
app/src/main/webapp/result.jsp (1)
1-86: PR 요청에 따라 리뷰 제외해당 JSP는 테스트용이며 리뷰 제외 요청을 반영해 코멘트를 생략합니다.
app/src/main/java/com/techcourse/controller/Step2Controller.java (1)
11-33: 작성자 요청에 따라 이 파일은 리뷰에서 제외합니다.테스트용 파일로 명시되어 있어 상세 리뷰는 생략합니다. 필요 시 별도 요청 주세요.
mvc/src/main/java/com/interface21/webmvc/servlet/mvc/tobe/handler/adapter/AnnotationHandlerAdapter.java (1)
11-23: 핸들러 판별과 위임 로직은 간결하고 명확합니다.HandlerExecution만 지원하도록 한 결정이 구조를 단순화합니다.
HandlerExecution.invoke 과정에서 발생하는 InvocationTargetException의 cause를 로깅/전파하는 정책은 어디에서 처리하시나요? 예외 원인 노출 수준을 결정해 운영 기준을 맞춰주세요.
app/src/main/java/com/techcourse/HandlerMappingRegistry.java (1)
22-25: AnnotationHandlerMapping의 스캔 범위(basePackage) 미설정이 의도된 기본값인가요?ControllerScanner의 기본 동작(빈 배열 시 전체 스캔/미스캔)을 확인해 주세요. 특정 패키지로 한정하지 않으면 예기치 않은 빈 등록/충돌 가능성이 있습니다.
mvc/src/main/java/com/interface21/webmvc/servlet/mvc/tobe/handler/adapter/ManualHandlerAdapter.java (1)
17-25: 레거시 Controller 호환 로직이 깔끔합니다.redirect: 프리픽스 처리도 JspView에 위임되어 책임 분리가 잘 되어 있습니다.
...n/java/com/interface21/webmvc/servlet/mvc/tobe/handler/mapping/AnnotationHandlerMapping.java
Show resolved
Hide resolved
mvc/src/main/java/com/interface21/webmvc/servlet/mvc/tobe/handler/mapping/HandlerExecution.java
Show resolved
Hide resolved
- `method.setAccessible(true)` 추가하여 접근 제어 문제 해결 - 리플렉션 호출 과정에서 발생할 수 있는 잠재적 예외 방지 및 안정성 강화
| import jakarta.servlet.http.HttpServletResponse; | ||
|
|
||
| @Controller | ||
| public class Step2Controller { |
There was a problem hiding this comment.
실행 잘 되네요! 직접 controller와 jsp를 만들어 테스트해보신 것 좋네요!
| .filter(Objects::nonNull) | ||
| .findFirst() | ||
| .orElseThrow( | ||
| () -> new ServletException("No handler found for request URI : " + request.getRequestURI()) |
There was a problem hiding this comment.
- HandlerMappingRegistry::getHandler의 반환 타입을 Opitonal 대신 Object 사용, xxRegistry 클래스의 getXxx 메서드는 둘 다 없으면 내부에서 예외를 터트리도록 수정했습니다. Optional로 구현하면, 존재하지 않을 경우의 예외 처리를 DispatcherServlet에서 해줘야 하는데 위와 마찬가지로 이것도 DispatcherServelt의 관심사가 아니라고 판단했습니다.
라고 하셨는데 핸들러 없음(404)과 어댑터 없음(500) 은 다른 문제 아닐까요?
getHandlerAdapter() 실패 (어댑터 없음) 가 발생한 시점은 이미 유효한 핸들러를 찾은 후입니다. 그럼에도 어댑터가 없다는 것은 서버 설정 오류가 맞기 때문에 오류를 던져 요청을 즉시 중단시키고 노랑이 한대로 500 응답을 던지는 게 맞을 것 같습니다.
그러나 getHandler() 실패 (핸들러 없음) 가 발생한 경우 클라이언트(사용자)가 존재하지 않는 URL을 요청한 경우입니다. 이는 서버의 버그가 아닌 정상적인 실패이지 않을까요? 현재 코드상으로는 바로 ServletException를 던지는데 이 경우 상위의 서블릿 컨테이너가 내부 오류로 판단해 곧바로 500 응답을 생성해서 클라이언트에게 보내주지 않을까요?
이 때, 클라이언트에게 404 응답을 보내려면 어떻게 해야 할까요?
There was a problem hiding this comment.
너무 좋은 리뷰네요! 예외 처리까진 생각하지 못했었습니다.
NoHandlerFoundException을 던지면 DispatcherServlet에서 404로 에러 응답을 보내주도록 처리했습니다!
|
|
||
| private Object instantiateController(Class<?> controllerClass) { | ||
| try { | ||
| return controllerClass.getDeclaredConstructor().newInstance(); |
There was a problem hiding this comment.
getDeclaredConstructor를 통해 private 생성자를 가져와도 setAccesible(true) 처리가 없기 때문에 private 생성자는 생성 못하지 않을까요?
There was a problem hiding this comment.
앗 이 부분을 자꾸 놓치네요! 꼼꼼한 리뷰 감사합니다 👀
| manualHandlerMapping.initialize(); | ||
| adHandlerMapping(manualHandlerMapping); | ||
|
|
||
| final var annotationHandlerMapping = new AnnotationHandlerMapping(); |
There was a problem hiding this comment.
AnnotationHandlerMapping 의 생성자는 basePackage를 설정할 수도 있는데 여기서 basePackage를 설정하지 않은 건 어떤 이유 때문인가요?
There was a problem hiding this comment.
예리한 질문이네요ㅎㅎ
basePackage 를 설정하지 않은 이유
프레임워크 영역에서 어플리케이션의 상세 구현 여부를 알게 되는 것이라고 판단해서 설정하지 않았습니다.
지금은 구조의 한계상 HandlerMappingRegistry가 app 패키지에 속하지만, 결국 mvc 패키지에 속하도록 하는 것이 최종 구조라고 생각했어요!
즉 HandlerMappingRegistry는 mvc 프레임워크의 영역인데, 애플리케이션 영역인 com.techcourse를 알고 있고, 설정하는 것이 어색하다고 판단해서 설정하지 않았습니다.
실제 Springboot mvc 프레임워크는?
실제 springboot mvc 프레임워크에서는 @ComponentScan 어노테이션을 사용해서 reflection을 사용할 basePackage를 지정한다고 합니다. @SpringBootApplication 어노테이션에 @ComponentScan이 포함되어 있어서, 어플리케이션 클래스가 속한 패키지와 그 하위 패키지를 스캔 대상으로 설정하는 방법을 씁니다.
중간 타협점
고민을 하다가 미션을 위한 특수한 상황이라고 가정하고 패키지 명을 추가했습니다ㅎㅎ
df38e0b
컴포넌트 스캔 기능 까지 구현한다면 best 겠지만 시간상 무리가 있어서 중간 타협 지점으로 패키지 명을 설정했습니다 👍
There was a problem hiding this comment.
프레임워크 영역에서 어플리케이션의 상세 구현 여부를 알지 못하게 하려 한 노랑의 의도가 너무 좋네요.
단순히 패키지명을 바로 쓰는 것과 , 위와 같은 생각을 했지만, 미션을 위한 특수 상황임을 인정하고 패키지명을 쓰는 건 천지차이라고 생각해요.
무엇이 더 나은 설계인지 명확히 인지하고 그걸 설명해주신 덕분에 많이 배우고 갑니다!
| import java.util.List; | ||
| import java.util.Objects; | ||
|
|
||
| public class HandlerMappingRegistry { |
There was a problem hiding this comment.
Registry의 책임과 DispatcherServlet::init
DispatcherServlet에 너무 많은 책임이 있는 것 같아서 각 Registry 클래스가 등록 목록들을 관리하도록 변경했습니다.
라고 하셨는데 DispatcherServlet의 책임이 과도하다고 느끼고 변경한 건 객체지향적으로 고민 끝에 나온 좋은 설계 같아요.
그런데 현재 구조는 Registry가 ManualHandlerMapping이나 AnnotationHandlerAdapter 같은 구체적인 클래스를 직접 생성(new)하고 등록하는 책임까지 갖게 되었는데요
만약 기존 레거시 컨트롤러를 제거하거나 새로운 컨트롤러를 추가하려면 HandlerMappingRegistry와 HandlerAdapterRegistry 양쪽의 내부 코드를 모두 수정해야 하는데
이는 곧 새로운 컨트롤러를 도입할 때 Registry 내부 구현을 알아야 한다는 뜻 아닐까요?
그리고 DispatcherServlet 에 책임이 과도하게 몰린다면 DispatcherInitializer에 생성하고 등록하는 책임을 맡길 수 있지 않을까요?
There was a problem hiding this comment.
내부 구현을 알아야 함
이는 곧 새로운 컨트롤러를 도입할 때 Registry 내부 구현을 알아야 한다는 뜻 아닐까요?
맞습니다. 지금 구조도 역시 문제가 있는 구조입니다 😂
미션 코드의 구조 상 새로운 종류의 Handler가 등장하면 내부 코드를 건드릴 수 밖에 없습니다!
- HandlerMapping과 HandlerAdapter 생성 책임을
DispatcherServlet에 둘 경우DispatcherServlet을 수정해야 합니다. - HandlerMapping과 HandlerAdapter 생성 책임을 각각의
Registry에 둘 경우 각Registry를 수정해야 합니다.
어느쪽에 등록되든 추가되어야 하는 코드의 양은 동일할 것 같습니다. DispatcherServlet에서 다 처리한다면 한 곳에서 수정할 수 있어서 장점이 있고, 각각의 Registry에서 처리한다면 등록을 할 때 각 책임에 맞는 부분만 신경쓰면 된다는 점에서 장점이 있습니다.
저는 후자의 장점이 더 크다고 판단해서 생성 책임을 분리했습니다!
DispatcherInitializer
DispatcherInitializer에 생성하고 등록하는 책임을 맡길 수 있지 않을까요?
해당 객체는 DispatcherServlet 을 초기화 하는 클래스이기 때문에 HandlerMapping, HandlerAdapter 에 대한 책임이 필요 없다고 판단했습니다.
그럼 Springboot MVC는?
그럼 실제 스프링부터 프레임워크에서 핸들러 매핑은 어떻게 불러올까요?! 이 부분이 궁금해져서 찾아봤습니다.
DispatcherServlet의 initHandlerMappings라는 메서드를 보면 다음과 같이 BeanFactoryUtils에서 가져오고 있습니다. 즉 HandlerMapping이 Bean으로 등록되어 있습니다.
private void initHandlerMappings(ApplicationContext context) {
this.handlerMappings = null;
if (this.detectAllHandlerMappings) {
// Find all HandlerMappings in the ApplicationContext, including ancestor contexts.
// *** HandlerMapping 클래스 타입으로 빈을 찾아온다 ***
Map<String, HandlerMapping> matchingBeans =
BeanFactoryUtils.beansOfTypeIncludingAncestors(context, HandlerMapping.class, true, false);
if (!matchingBeans.isEmpty()) {
this.handlerMappings = new ArrayList<>(matchingBeans.values());
// We keep HandlerMappings in sorted order.
AnnotationAwareOrderComparator.sort(this.handlerMappings);
}
}
}그럼 빈은 어디서 등록이 되는걸까요? 또 찾아보니까 WebMvcConfigurationSupport에서 빈으로 등록해주고 있습니다. 그런데 신기한 부분은 모든 HandlerMapping, HandlerAdapter를 직접 등록하고 있더라구요!!
public class WebMvcConfigurationSupport implements ApplicationContextAware, ServletContextAware {
...
@Bean
public RequestMappingHandlerMapping requestMappingHandlerMapping(...) {
// @RequestMapping을 처리하는 HandlerMapping을 생성하고 빈으로 등록
RequestMappingHandlerMapping mapping = createRequestMappingHandlerMapping();
// ... 여러 설정 적용 ...
return mapping;
}
@Bean
public HandlerMapping viewControllerHandlerMapping(...) {
// 특정 URL 경로를 View 이름에 직접 매핑하는 HandlerMapping 생성
// ...
}
@Bean
public HandlerMapping resourceHandlerMapping(...) {
// 정적 리소스를 처리하는 HandlerMapping 생성
// ...
}
...즉 스프링 프레임워크 내부에서도, 새로운 종류의 Handler가 추가되면 코드를 수정해야 하는 구조였습니다...! 신기하네요ㅎㅎ
Best Practice
새롭게 알게 된 내용을 바탕으로 기존 제가 구현한 코드를 리팩터링 한다면 Factory 클래스를 하나 작성해서 Handler 관련 객체들의 생성 책임을 한 군데로 모으는 것도 좋을 것 같습니다. 다만 시간 관계상 여력이 된다면 다음 단계에서 같이 고민해 보겠습니다 🫡
There was a problem hiding this comment.
관련해서 블로그 글 하나 작성했어요! 심심할 때 읽어보세요ㅎㅎ
https://yesjuhee.tistory.com/23
There was a problem hiding this comment.
와우 정말 감탄했습니다. 😮 노랑의 정리한 글과 블로그 글 덕분에 정말 많이 배우고 갑니다.
노랑이 보여준 factory 클래스를 보고 나니
스프링은 '새로운 핸들러가 추가될 때 내 코드 수정을 없앨 수 있을까?' 라는 질문에 답하려 한 게 아닌
'새로운 핸들러 추가와 같은' '변경'이 발생했을 때, 코드의 단 한 곳인 가장 예측 가능하고 안전한 단일 지점에서만 수정이 일어나도록 설계할 수 있을까?'
라는 질문에 답을 하기 위한 코드였다는 생각이 드네요.
또 스프링에서 Reflections를 이용한 완전 자동 스캔 방식을 사용하지 않았다면 어떤 트레이드오프가 있길래 스프링은 선택하지 않았을지도 궁금해지네요.
클래스패스를 모두 스캔하는 데 드는 성능 비용 때문? 어떤 빈이 등록될지 예측하기 어렵다?
이 주제에 대해서도 나중에 이야기 나눠보면 정말 재미있을 것 같습니다. 깊이 있는 분석과 공유 정말 고마워요! 👍
- 요청 처리 시 핸들러를 찾지 못하는 경우를 처리하기 위해 `NoHandlerFoundException` 클래스 추가 - `DispatcherServlet`에서 `NoHandlerFoundException` 발생 시 대응 로직 추가 - HTTP 404 에러 응답을 반환하는 `noHandlerFound` 메서드 추가 - 기존 코드에서 일반 `ServletException` 대신 `NoHandlerFoundException` 사용하도록 수정하여 예외 메시지 명확화
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (10)
mvc/src/main/java/com/interface21/webmvc/servlet/NoHandlerFoundException.java (3)
5-5: 직렬화 UID 고려(선택)ServletException은 Serializable 계층에 속합니다. 예외 객체가 세션 직렬화 경로에 실릴 가능성을 고려하면 serialVersionUID를 명시해두는 것도 한 방법입니다(필수는 아님).
7-10: 원본 httpMethod/requestUri를 예외에 보존하고 접근자 추가
- mvc/src/main/java/com/interface21/webmvc/servlet/NoHandlerFoundException.java — 생성자에서 httpMethod와 requestUri를 필드로 저장하고 public getter 제공(디버깅/로깅 재사용).
- 호출부 확인: app/src/main/java/com/techcourse/HandlerMappingRegistry.java:34에서 request.getRequestURI()를 전달하므로 "URI(경로)"입니다. 필드명·문서화는 requestUri로 통일하고, getRequestURI()의 디코딩 동작(컨테이너별 차이)을 로그 저장 시 고려하세요.
5-11: 용어 일관성 — 'handler'로 통일 권장DispatcherServlet 로그와 HandlerAdapter에서 'handler' 용어가 사용되고 있으므로 NoHandlerFoundException의 예외 메시지("No endpoint …")를 "No handler …"로 변경해 코드베이스 전체의 용어를 통일하세요.
- mvc/src/main/java/com/interface21/webmvc/servlet/NoHandlerFoundException.java:8 — 현재: super("No endpoint " + httpMethod + " " + requestURL + ".");
- app/src/main/java/com/techcourse/DispatcherServlet.java:40 — 로그: log.warn("No Handler Found Exception : {}", e.getMessage());
- mvc/src/main/java/com/interface21/webmvc/servlet/mvc/tobe/handler/adapter/HandlerAdapterRegistry.java:24 — 예외: new ServletException("No handler adapter")
대소문자·구두점 표기도 함께 일관되게 맞춰 커밋하세요.
app/src/main/java/com/techcourse/DispatcherServlet.java (7)
19-20: 필드 명명 일관성(오탈자 수준): handlermappingRegistry → handlerMappingRegistry카멜 케이스가 깨져 가독성이 떨어집니다. 클래스명과 동일한 단어 경계를 맞춰 주세요.
22-25: 생성자에서 직접 new: 테스트 용이성과 확장성 제약레지스트리를 내부에서 직접 생성하면(하드 와이어링) 대체/모킹이 어려워집니다. 구성 책임을 외부로 밀어내거나(주입/팩토리) 테스트 전용 생성자 등 확장 포인트를 고려해보셨나요?
28-33: 메서드 길이 규칙(≤10줄) 살짝 초과service는 예외 처리와 디스패치 책임이 섞여 줄 수가 늘었습니다. 예외 처리/디스패치/렌더링을 더 작게 나누면(의도 단위 메서드 추출) 규칙 1·6을 함께 충족시킬 수 있습니다.
32-32: 로그 컨텍스트 보강(선택)요청 URI와 함께 쿼리스트링, 추적 ID(있다면) 등을 포함하면 장애 재현이 쉬워집니다. 로그 키의 레이블(“Method :”) 스페이싱도 프로젝트 표준에 맞춰 주세요.
56-61: Tell, Don’t Ask 관점에서의 렌더링 책임 이동 검토현재 ModelAndView에서 getView/getModel을 꺼내와 외부에서 렌더링합니다. 렌더링 책임을 View 또는 ModelAndView 쪽으로 이동시키면 게터 의존을 줄이고(규칙 9) 확장지점을 한곳으로 모을 수 있습니다. “만약 새로운 응답 형식이 늘어난다면 어디를 바꾸게 될까요?”를 자문해보세요.
14-62: 회고 질문
- 이번 단계에서 “핸들러 탐색”과 “어댑터 실행”의 경계를 어디까지 고정할지, 향후 인터셉터/예외리졸버/뷰리졸버를 넣을 여지를 어떻게 남겨두실지 고민해보셨나요?
- “어댑터 미존재”는 500, “핸들러 미존재”는 404로 구분되어 있나요? 관찰 가능성(메트릭/로그 키) 관점에서 어떻게 드러날까요?
원하시면 테스트 시나리오 목록(케이스 명세)과 체크리스트 형태로 도와드릴게요.
35-38: 핸들러 어댑터 handle()의 non-null 반환 계약 문서화
ManualHandlerAdapter, AnnotationHandlerAdapter, HandlerExecution 모두 ModelAndView를 직접 생성·반환하므로 현재는 null 반환 케이스가 없습니다. 향후 안전성을 위해 @nonnull 애노테이션 또는 문서에 “always non-null” 계약을 명시하고, 필요시 Objects.requireNonNull 검증 로직을 추가하세요.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
app/src/main/java/com/techcourse/DispatcherServlet.java(1 hunks)app/src/main/java/com/techcourse/HandlerMappingRegistry.java(1 hunks)mvc/src/main/java/com/interface21/webmvc/servlet/NoHandlerFoundException.java(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- app/src/main/java/com/techcourse/HandlerMappingRegistry.java
🧰 Additional context used
📓 Path-based instructions (1)
**/*.java
⚙️ CodeRabbit configuration file
**/*.java: ## 🎯 코드 품질 중심 리뷰 가이드라인이 리뷰는 코드 품질과 객체지향 원칙에 집중합니다.
미션 달성 여부가 아닌, 코드 설계와 품질 개선에 대한 피드백을 한글로 제공해주세요.📚 학습 원칙
- 직접 코드를 제공하지 마세요 (학습자가 명시적으로 요청하는 경우 제외)
- 문제 해결 과정을 안내하되, 정답을 바로 알려주지 마세요
- 작은 단계로 문제를 분해하여 접근하도록 도와주세요
💡 피드백 방법
- 유도 질문 활용: "만약 ~라면 어떻게 될까요?", "~를 고려해보셨나요?"
- 힌트 제공: 방향은 제시하되, 구체적인 구현은 학습자가 하도록
- 다양한 접근법 제시: 한 가지 해결책이 아닌 여러 가능성을 제안
- 왜?에 집중: 단순히 무엇이 잘못되었는지보다 왜 그런지 이해하도록
⚡ 객체지향 생활 체조 원칙 검토
다음은 객체지향 생활 체조(Object Calisthenics) 9가지 원칙입니다.
위반 시 학습 효과를 위해 반드시 피드백을 제공하되, 왜 이 원칙이 중요한지 설명해주세요:규칙 1: 한 메서드에 오직 한 단계의 들여쓰기만
- 들여쓰기 depth 최대 1 (중첩 제어구조 금지)
- 📖 이유: 메서드 복잡도 감소, 단일 책임 원칙 강화
- 💡 힌트: "이 부분을 별도 메서드로 추출하면 어떨까요?"
규칙 2: else 예약어 금지
- else, switch/case 사용 금지
- 📖 이유: 복잡한 분기 제거, 명확한 코드 흐름
- 💡 힌트: "early return이나 가드 클로즈 패턴을 고려해보세요"
규칙 3: 모든 원시값과 문자열 포장
- 원시 타입과 String을 객체로 포장
- 📖 이유: 도메인 개념 명확화, 비즈니스 로직 응집
- 💡 예시:
int port→Port port,String name→Name name규칙 4: 한 줄에 점 하나만 (디미터 법칙)
- 메서드 체이닝 제한
- 📖 이유: 결합도 감소, 캡슐화 향상
- 💡 나쁜 예:
request.getUri().getPath().substring()- 💡 좋은 예:
request.extractPath()규칙 5: 축약 금지
- 명확한 이름 사용 (축약어 금지)
- 📖 이유: 코드 가독성, 의도 명확화
- 💡 예시:
req→request,calcAmt→calculateAmount규칙 6: 모든 엔티티를 작게 유지
- 클래스 50줄, 메서드 10줄 이하
- 📖 이유: 단일 책임, 이해와 테스트 용이성
- 💡 힌트: "이 클래스가 너무 많은 일을 하고 있지 않나요?"
규칙 7: 인스턴스 변수 3개 이하
- 클래스당 최대 3개의 인스턴스 변수
- 📖 이유: 높은 응집도, 단일 책임 유지
- 💡 힌트: "관련 필드들을 별도 객체로 묶을 수 있을까요?"
규칙 8: 일급 컬렉션 사용
- 컬렉션을 감싸는 클래스 사용
- 📖 이유: 컬렉션 로직 캡슐화, 불변성 보장
- 💡 예시:
List<HttpHeader>→HttpHeaders클래스규칙 9: 게터/세터/프로퍼티 금지
- Tell, Don't Ask 원칙 준수
- 📖 이유: 진정한 캡슐화, 객체 자율성
- 💡 나쁜 예:
if (user.getPassword().equals(input))- 💡 좋은 예:
if (user.authenticate(input))🎯 추가 프로그래밍 요구사항
코드 스타일
- Java Style Guide 준수
- 3항 연산자 사용 금지
테스트 및 구조
- **모든 기능 T...
Files:
mvc/src/main/java/com/interface21/webmvc/servlet/NoHandlerFoundException.javaapp/src/main/java/com/techcourse/DispatcherServlet.java
🧬 Code graph analysis (1)
app/src/main/java/com/techcourse/DispatcherServlet.java (3)
mvc/src/main/java/com/interface21/webmvc/servlet/ModelAndView.java (1)
ModelAndView(7-33)mvc/src/main/java/com/interface21/webmvc/servlet/NoHandlerFoundException.java (1)
NoHandlerFoundException(5-11)mvc/src/main/java/com/interface21/webmvc/servlet/mvc/tobe/handler/adapter/HandlerAdapterRegistry.java (1)
HandlerAdapterRegistry(8-31)
🔇 Additional comments (1)
app/src/main/java/com/techcourse/DispatcherServlet.java (1)
39-41: 404 처리 분리 — 설계 적합; 통합 테스트 확인 필요NoHandlerFoundException을 catch해 404를 명시적으로 반환하는 흐름은 책임 분리가 잘 되어 있습니다. 향후 JSON/API vs JSP 등 공통 에러 응답을 전략 객체로 위임하는 방안도 고려하세요.
위치: app/src/main/java/com/techcourse/DispatcherServlet.java (39–41)
요청: 404 경로에 대한 통합 테스트가 있는지 확인해 주세요 — 자동 검색 결과 app/src/test/java 디렉터리가 없어 검증할 수 없었습니다.
- 기본 생성자 호출 시 `setAccessible(true)` 추가하여 리플렉션 호출 안정성 강화 - `getDeclaredConstructor` 메서드 분리로 코드 가독성 및 재사용성 향상 - 예외 메시지 및 로그를 통해 디버깅 용이성 확보 - 불필요한 개행 제거로 코드 정리
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
mvc/src/main/java/com/interface21/webmvc/servlet/mvc/tobe/handler/mapping/ControllerScanner.java (5)
29-37: Map 노출 대신 일급 컬렉션으로 의도 드러내기원시 Map을 반환하면 이후 단계에서 조작·오용 여지가 큽니다. Controllers 같은 일급 컬렉션으로 감싸 책임(중복 키 방지, 정렬/우선순위, 불변성)을 캡슐화해보면 어떨까요? (규칙 8: 일급 컬렉션, 규칙 9: Tell, Don’t Ask)
39-51: 예외 처리·로깅 전략 정교화
- log.error 후 즉시 RuntimeException 재던지기는 상위에서 다시 로깅되어 “이중 로그”가 되기 쉽습니다. 한 곳에서만 기록하도록 책임을 정리해보면 어떨까요?
- 광범위한 Exception 캐치는 원인 분류를 어렵게 합니다. 반면 NoSuchMethodException만 별도 처리 중인데, 나머지도 의도에 따라 구체화(예: 접근 권한, 호출 실패)해보면 디버깅이 쉬워집니다.
- 도메인 특화 예외(예: ControllerInstantiationException)로 의미를 명확히 해보면 호출측에서의 복구/메시지 처리도 간결해집니다.
39-44: 수명주기·스레드 안전성 가정 명시 필요현재 컨트롤러를 스캔 즉시 인스턴스화해 Map에 담는 구조는 사실상 싱글턴 스코프입니다. 상태ful 컨트롤러가 등장하면 동시성 문제가 생길 수 있는데,
- 컨트롤러는 “무상태(Stateless)”여야 한다는 제약을 문서화할까요?
- 또는 인스턴스화 전략(요청당, 프로토타입, 팩토리/DI 훅)을 열어 확장 가능하게 할까요?
45-49: 로그 메시지 콘텍스트 보강“컨트롤러 기본 생성자가 필요합니다”처럼 원인 메시지에 controllerClass의 FQCN 등 콘텍스트를 포함하면 장애 원인 추적이 빨라집니다. 또한 다국어/외부화 전략을 고려해보면 운영 환경 메시지 일관성도 향상됩니다.
53-55: 헬퍼 메서드의 역할 재검토getDeclaredConstructor는 현재 한 곳에서만 호출되고 단순 위임입니다.
- 가독성 향상에 기여한다면 유지,
- 아니라면 인라인해 복잡도와 노출 표면을 줄이는 것도 방법입니다.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
mvc/src/main/java/com/interface21/webmvc/servlet/NoHandlerFoundException.java(1 hunks)mvc/src/main/java/com/interface21/webmvc/servlet/mvc/tobe/handler/mapping/ControllerScanner.java(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- mvc/src/main/java/com/interface21/webmvc/servlet/NoHandlerFoundException.java
🧰 Additional context used
📓 Path-based instructions (1)
**/*.java
⚙️ CodeRabbit configuration file
**/*.java: ## 🎯 코드 품질 중심 리뷰 가이드라인이 리뷰는 코드 품질과 객체지향 원칙에 집중합니다.
미션 달성 여부가 아닌, 코드 설계와 품질 개선에 대한 피드백을 한글로 제공해주세요.📚 학습 원칙
- 직접 코드를 제공하지 마세요 (학습자가 명시적으로 요청하는 경우 제외)
- 문제 해결 과정을 안내하되, 정답을 바로 알려주지 마세요
- 작은 단계로 문제를 분해하여 접근하도록 도와주세요
💡 피드백 방법
- 유도 질문 활용: "만약 ~라면 어떻게 될까요?", "~를 고려해보셨나요?"
- 힌트 제공: 방향은 제시하되, 구체적인 구현은 학습자가 하도록
- 다양한 접근법 제시: 한 가지 해결책이 아닌 여러 가능성을 제안
- 왜?에 집중: 단순히 무엇이 잘못되었는지보다 왜 그런지 이해하도록
⚡ 객체지향 생활 체조 원칙 검토
다음은 객체지향 생활 체조(Object Calisthenics) 9가지 원칙입니다.
위반 시 학습 효과를 위해 반드시 피드백을 제공하되, 왜 이 원칙이 중요한지 설명해주세요:규칙 1: 한 메서드에 오직 한 단계의 들여쓰기만
- 들여쓰기 depth 최대 1 (중첩 제어구조 금지)
- 📖 이유: 메서드 복잡도 감소, 단일 책임 원칙 강화
- 💡 힌트: "이 부분을 별도 메서드로 추출하면 어떨까요?"
규칙 2: else 예약어 금지
- else, switch/case 사용 금지
- 📖 이유: 복잡한 분기 제거, 명확한 코드 흐름
- 💡 힌트: "early return이나 가드 클로즈 패턴을 고려해보세요"
규칙 3: 모든 원시값과 문자열 포장
- 원시 타입과 String을 객체로 포장
- 📖 이유: 도메인 개념 명확화, 비즈니스 로직 응집
- 💡 예시:
int port→Port port,String name→Name name규칙 4: 한 줄에 점 하나만 (디미터 법칙)
- 메서드 체이닝 제한
- 📖 이유: 결합도 감소, 캡슐화 향상
- 💡 나쁜 예:
request.getUri().getPath().substring()- 💡 좋은 예:
request.extractPath()규칙 5: 축약 금지
- 명확한 이름 사용 (축약어 금지)
- 📖 이유: 코드 가독성, 의도 명확화
- 💡 예시:
req→request,calcAmt→calculateAmount규칙 6: 모든 엔티티를 작게 유지
- 클래스 50줄, 메서드 10줄 이하
- 📖 이유: 단일 책임, 이해와 테스트 용이성
- 💡 힌트: "이 클래스가 너무 많은 일을 하고 있지 않나요?"
규칙 7: 인스턴스 변수 3개 이하
- 클래스당 최대 3개의 인스턴스 변수
- 📖 이유: 높은 응집도, 단일 책임 유지
- 💡 힌트: "관련 필드들을 별도 객체로 묶을 수 있을까요?"
규칙 8: 일급 컬렉션 사용
- 컬렉션을 감싸는 클래스 사용
- 📖 이유: 컬렉션 로직 캡슐화, 불변성 보장
- 💡 예시:
List<HttpHeader>→HttpHeaders클래스규칙 9: 게터/세터/프로퍼티 금지
- Tell, Don't Ask 원칙 준수
- 📖 이유: 진정한 캡슐화, 객체 자율성
- 💡 나쁜 예:
if (user.getPassword().equals(input))- 💡 좋은 예:
if (user.authenticate(input))🎯 추가 프로그래밍 요구사항
코드 스타일
- Java Style Guide 준수
- 3항 연산자 사용 금지
테스트 및 구조
- **모든 기능 T...
Files:
mvc/src/main/java/com/interface21/webmvc/servlet/mvc/tobe/handler/mapping/ControllerScanner.java
🔇 Additional comments (1)
mvc/src/main/java/com/interface21/webmvc/servlet/mvc/tobe/handler/mapping/ControllerScanner.java (1)
41-43: 기존 피드백 반영 확인 — private 생성자 접근 처리됨setAccessible(true) 추가로 private 기본 생성자 인스턴스화 문제를 해결하셨네요. 이전 코멘트의 우려가 해소되었습니다.
...src/main/java/com/interface21/webmvc/servlet/mvc/tobe/handler/mapping/ControllerScanner.java
Show resolved
Hide resolved
...src/main/java/com/interface21/webmvc/servlet/mvc/tobe/handler/mapping/ControllerScanner.java
Show resolved
Hide resolved
|
벡터 리뷰 너무 좋은데요?! 생각해 볼 것이 많아서 많이 배웠습니다! |
Byesol
left a comment
Byesol
left a comment
There was a problem hiding this comment.
고생하셨어요~ 노랑!
좋은 블로그 글 잘 읽었습니다! 덕분에 많이 배웠어요.
이만 merge 하도록 할게요. 다음 단계에서 만나요~~! ┌(・。・)┘♪
2 participants
기능 요구사항 구현 & 테스트
Controller 인터페이스 기반의 레거시 MVC와, 1단계에서 추가된 @controller 어노테이션 기반의 MVC가 함께 작동하는 구조를 구현했습니다.
해당 기능을 실제로 테스트 해보기 위해
Step2Controller컨트롤러를 추가했습니다. 서버 실행하고localhost:8080/form으로 요청을 보내면 해당 컨트롤러 로직을 테스트할 수 있습니다!아래 3개 파일은 테스트용으로 추가한거라 리뷰에서 제외하고 보시면 됩니다
app/src/main/webapp/result.jspapp/src/main/webapp/form.jspapp/src/main/java/com/techcourse/controller/Step2Controller.java추가된 클래스
기존 기능을 유지하면서, 신규 기능을 추가하기 쉬운 구조를 만들기 위해 LMS에서 제시된 구조를 최대한 따라가는 방식으로 진행했습니다.
ControllerScanner
기존
AnnotationHandlerMapping에서 하던 일부 작업을 가져갔습니다. 크게 필요성을 느끼진 못했는데 일단 LMS 구조를 최대한 따라가보는 방향으로 진행했습니다.HandlerMapping, HandlerMappingRegistry
AnnotationHandlerMapping,ManualHandlerMapping처럼 Http Request에 해당하는 적절한 헨들러를 반환하는 클래스를HandlerMapping인터페이스로 추상화하여 관리합니다.MVC에서 사용되는 모든 HandlerMapping을
HandlerMappingRegistry의 생성자에서 초기화하여 관리하고 있습니다.HandlerAdapter, HandlerAdapterRegistry
핸들러의 종류에 따라서 작동 방식이 다르기 때문에 이를 추상화 하기 위해
HandlerAdapter인터페이스를 도입했습니다. 해당 인터페이스는org.springframework.web.servlet.HandlerAdapter를 참고했습니다.MVC에서 사용되는 모든 HandlerMapping을
HandlerAdapterRegistry의 생성자에서 초기화하여 관리하고 있습니다.임의로 변경한 부분
주어진 클래스 다이어그램을 최대한 따라가긴 했는데, 몇가지 잘 이해가 안되는 부분은 임의로 변경했습니다.
DispatcherServlet::init메서드 삭제HandlerAdapterRegistry::addHandlerAdapter,HandlerMappingRegistry::addHandlerAdapter메서드를 private 으로 변경해서 사용처음에는 기존 구조에 최대한 맞춰서
DispatcherServlet::init메서드에서addXxx를 써서xxxRegistry클래스에 객체들을 추가하는 방식으로 구현했습니다. 그런데 DispatcherServlet에 너무 많은 책임이 있는 것 같아서 각 Registry 클래스가 등록 목록들을 관리하도록 변경했습니다. 그래서DispatcherServlet::init메서드가 필요 없어졌고,addXxx메서드도 내부에서만 사용하기 때문에 private 으로 변경했습니다.생성에 대한 책임은 최대한 생성자에 있는걸 선호하는 편인데, 이번 뼈대 코드는 거의 빈 생성자를 사용해서 좀 의아하게 생각하고 있어요ㅎㅎ
HandlerMappingRegistry::getHandler의 반환 타입을Opitonal<Object>대신Object사용xxRegistry클래스의getXxx메서드는 둘 다 없으면 내부에서 예외를 터트리도록 수정했습니다. Optional로 구현하면, 존재하지 않을 경우의 예외 처리를DispatcherServlet에서 해줘야 하는데 위와 마찬가지로 이것도DispatcherServelt의 관심사가 아니라고 판단했습니다.혹시 이와 관련해서 추가적인 의견 있으면 듣고 싶어요 😄