Feat : url을 통한 뉴스 크롤링 및 HCX를 통한 뉴스 리포트 생성#7
Conversation
Test Results1 tests 1 ✅ 0s ⏱️ Results for commit 5e3590e. |
There was a problem hiding this comment.
전체적으로 크롤링 로직이나 셀렉터 처리, 컨버터 등 잘 구현해주셨습니다.
사실 크롤링 로직은 제가 완전히 이해할 수 없어서.. 구조적인 부분과 예외 처리에대한 부분 위주로 보았습니다. 아래와 같은 부분들은 리팩토링하면 더 구조적인 코드가 될 것 같고 가독성을 높일 수 있을 것 같아 코멘트 드립니다.
1. Converter는 변환 책임만 갖도록 분리 필요
현재 컨버터 클래스에서 JSON 문자열 생성이나 텍스트 escaping 등 비즈니스 로직이 일부 포함되어 있습니다. Converter는 Entity → DTO, 또는 내부 객체 간 변환에 집중하고
JSON 변환이나 문자열 처리 로직은 별도의 Util 클래스로 분리하는 것이 더 명확하고 유지보수에도 유리합니다.
2. 예외 처리: Handler + Status 조합 방식 사용 권장
현재 catch 블록에서 단순 문자열을 반환하거나 GeneralException을 직접 던지는 방식이 사용되고 있습니다.
이 프로젝트에서는 도메인별 Handler 클래스 (예: NewsHandler, ReportHandler)에서 GeneralException을 상속받고,
예외 발생 시 Handler와 해당 도메인의 ErrorStatus를 함께 사용하여 예외를 던지는 구조를 사용하는 것이 좋을 것 같습니다.
이렇게 하면 예외 상황에서도 클라이언트에게 일관된 JSON 응답을 줄 수 있고, 로깅 및 예외 메시지를 중앙화할 수 있어 구조적으로 더 안정적인 설계가 됩니다.
또한 단순 문자열 반환보다는 명시적인 예외를 통해 예외 흐름을 관리하는 것이 더 명확한 방식입니다.
3. 클라이언트 응답용 DTO에 명세 추가
컨트롤러에서 반환하는 응답 DTO (NewsArticleResponse 등)에 필드가 어떤 의미인지 명세가 부족합니다.각 필드가 어떤 데이터를 나타내는지 어떤 형식인지에 대한 설명을
Swagger의 @Schema(description = "...") 어노테이션을 통해 추가해주시면
프론트엔드 분들이 api에 대해 이해하기 쉬울 것 같습니다.
4. 기타 사항
selectorConfig 필드 선언부에 패키지 전체 경로를 직접 작성하신 부분이 있는데, 특별한 이유가 없다면 import 처리 후 클래스명만 사용해 주시면 좋을 것 같습니다. catch 블록에서 예외 로깅 없이 문자열만 반환하는 경우가 있는데 로그도 남기지 않으면 운영 중 문제 파악이 어렵습니다. Handler 처리를 해주시거나, 최소한 log.warn 또는 log.error 수준의 로그는 남겨주시면 좋겠습니다.
--
사실 저보다는 크롤링이나 비즈니스 로직에 대해 더 잘 알고 계실 것 같아서 리뷰를 남기는 게 조심스럽긴합니다.. 기능적으로는 잘 동작하는 코드일 것으로 보이고 크롤링 처리 자체는 무리 없이 구성되어 있다고 판단됩니다. 다만, 전체적으로 구조적인 부분에서 몇 가지 일관된 설계 방향을 적용해주시면 코드의 가독성, 유지보수성, 확장성 측면에서 더 좋아질 것 같아 코멘트 드렸습니다.
- Converter는 변환만 담당하도록 책임 분리
- 예외는 Handler + ErrorStatus 조합으로 명확하게 관리
- 응답 DTO에는 Swagger 명세 등을 통해 의미와 예시를 명확히 정의
- 불필요한 패키지 경로 제거 및 catch 블록 내 예외 로깅 보완
일단 시간이 부족한 상황이니 이번 PR은 머지하고,
언급드린 부분들에 대해서는 후속 리팩토링으로 꼭 반영해주시면 감사하겠습니다.
|
|
||
| private final NewsService newsService; | ||
|
|
||
| @GetMapping("/article-content") |
There was a problem hiding this comment.
현재 컨트롤러 메서드에 Swagger 명세가 누락되어 있습니다.
추후 @operation, @parameter 등의 어노테이션을 활용해 각 API의 목적, 요청 파라미터, 예시 값 등을 문서화해 주세요!
| * 뉴스 데이터를 NewsArticleResponse로 변환 | ||
| */ | ||
| public NewsArticleResponse toNewsArticleResponse(String title, String date, String content) { | ||
| try { |
There was a problem hiding this comment.
현재 NewsConverter는 단순히 DTO 변환을 넘어서 JSON 직렬화 및 문자열 이스케이프 처리까지 수행하고 있습니다. Converter는 객체 간 변환 ( Entity → DTO) 역할에 집중하고
직렬화/문자열 처리 로직은 별도의 유틸 클래스로 분리하는 것이 책임 분리에 더 적합하다고 생각합니다.
크롤링 특성상 이렇게 처리한 점은 이해되지만, 추후 리팩토링 시 Converter는 변환 책임만 갖도록 구조를 정리하는 것을 추천드립니다.!
| @Getter | ||
| @NoArgsConstructor | ||
| @AllArgsConstructor | ||
| public class NewsArticleResponse { |
There was a problem hiding this comment.
NewsController에서 프론트엔드에게 응답으로 전달되는 NewsArticleResponse는 DTO로 사용되고 있지만, 각 필드가 어떤 의미를 가지는지에 대한 명세나 설명이 작성되어 있지 않아 파악하기 어렵습니다.
프론트엔드에서 쉽게 이해하고 활용할 수 있도록
각 필드에 Swagger 명세(annotation)를 통해 어떤 정보를 담고 있는지 어떤 형식/예시의 값이 전달되는지 명확히 기재해주시면 좋을 것 같습니다.
나중에 소셜 로그인 쪽 컨트롤러 보시고 작성해주시면 좋을 것 같습니다.
| */ | ||
| public void handleContentNotFound(String url, String operation) { | ||
| log.warn("Content not found during {} for URL: {}", operation, url); | ||
| throw new GeneralException(NewsErrorStatus.NEWS_CONTENT_NOT_FOUND); |
There was a problem hiding this comment.
지금 코드에서는 GeneralException을 직접 던지는 방식으로 예외를 처리하고 있는데, 각 도메인 별로 Handler 클래스를 만들어서 GeneralException을 상속 받는 구조로 예외를 처리하는 방식을 추천합니다.
예를 들어서
throw new GeneralException(NewsErrorStatus.NEWS_NAVER_API_CALL_FAILED);
이런식으로 직접 예외를 던지기 보다는
public class NewsHandler extends GeneralException {
public NewsHandler(NewsErrorStatus code) {
super(code);
}
}
이런식으로 각 도메인마다 핸들러를 정의하고
throw new NewsHandler(NewsErrorStatus.NEWS_NAVER_API_CALL_FAILED);
예외를 던져야 하는 곳에서 에러 상태 코드와, 핸들러를 재사용하는 방식이 좋을 것 같습니다.
이런식으로 도메인 전용 핸들러 클래스를 만들어서 사용해주시면 좋을 것 같습니다.
| try { | ||
| return extractor.extract(); | ||
| } catch (Exception e) { | ||
| return null; |
There was a problem hiding this comment.
여기서도 마찬가지로 단순히 null을 반환하는 것보다 핸들러 + 에러 상태 코드로 예외 처리를 수행하는 것이 나중에 디버깅할떄 편할 것 같습니다.
|
|
||
| @Getter | ||
| @AllArgsConstructor | ||
| public enum NewsErrorStatus implements BaseErrorCode { |
There was a problem hiding this comment.
각 도메인마다 상태 코드를 정의해주신 점은 좋습니다!
앞서 남긴 코멘트와도 연결되는 부분인데,
예외를 발생시킬 때는 각 도메인별로 정의한 Handler 클래스에서 GeneralException을 상속받고
예외 상황이 발생한 곳에서는 해당 Handler와 도메인별 ErrorStatus를 사용해 예외를 던지는 방식을 추천드립니다.
이렇게 하면 예외 상황에서도 클라이언트에게 일관된 형태의 응답을 전달할 수 있고,
도메인별로 예외 처리를 구조적으로 분리할 수 있어 유지보수와 확장성 측면에서도 도움이 됩니다.
| public class DateExtractorServiceImpl implements DateExtractorService { | ||
|
|
||
| private final HtmlParserService htmlParserService; | ||
| private final com.perfact.be.domain.news.config.SelectorConfig selectorConfig; |
There was a problem hiding this comment.
SelectorConfig를 private final com.perfact...SelectorConfig처럼 패키지 전체 경로로 선언하신 부분이 보이는데요!
특별한 이유가 없다면 import를 추가하고 필드 선언은 private final SelectorConfig selectorConfig; 형태로 바꿔주세요..!
| } | ||
| } | ||
|
|
||
| return "날짜 정보 없음"; |
There was a problem hiding this comment.
전체적으로 catch 블록에서 "날짜 정보 없음"과 같은 단순 문자열만 반환하고 있는 부분이 여러 군데 보입니다.
이런 방식은 실제 예외 발생 원인을 숨기고,
클라이언트 측에서도 정확한 실패 이유를 알 수 없게 됩니다..
그리고 무엇보다 콘솔 로그 외에는 문제를 추적할 수 없기 때문에 운영 환경에서는 리스크가 큽니다...
혹시 특별한 이유가 없다면, 프로젝트 내 공통 예외 처리 설계(GeneralException + 도메인별 Handler + ErrorStatus)에 따라
catch 블록에서도 도메인 전용 핸들러를 활용해 예외를 명확하게 던져주는 방식으로 개선하는 것을 권장드립니다!
2 participants
관련 이슈
#6
작업한 내용
리포트 생성
POST /api/report
크롤링 (프론트에서 안쓰더라도 일단 따로 만들었습니다)
GET /api/news/article-content?url={뉴스링크}
네이버 뉴스 검색 (추후 프론트에서 확장 기능으로 사용 할 수 있을 것이고, 타 도메인 뉴스 매핑할 때 나중에 사용할 것 같아서 같이 추가했습니다)
GET /api/news/article-content?url={뉴스링크}
코드 수정 요청 부담없이 마구 부탁드립니다.. 일괄적으로 가지뻗기 식으로 해버리게 된 것 같아서.. 놓친 부분이 있다면 바로 칼수정 할게요!!
PR Point 및 참고사항, 스크린샷