Feature/member mypage #72 #75
Conversation
HiSeungmin
requested review from
Eseas,
taekminKwon,
K-KY,
kdh10806 and
jeongns2611
as code owners
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| pull_request: | ||
| branches: [ "dev", "main" ] | ||
| branches: [ "dev", "main", "setting/multi-module" ] | ||
|
|
||
| jobs: | ||
| build: |
There was a problem hiding this comment.
코드 패치에 대한 간단한 코드 리뷰를 진행하겠습니다.
변경 내용
push및pull_request이벤트에서 브랜치 목록에"setting/multi-module"이 추가되었습니다.
잠재적 버그 위험
- 브랜치 추가로 인한 영향:
"setting/multi-module"브랜치가 추가됨으로써, 이 브랜치에 대한 CI/CD 파이프라인이 예상치 못한 동작을 할 수 있습니다. 해당 브랜치에 적절한 테스트가 없거나 잘못된 코드가 포함될 경우, 의도치 않게 배포될 위험이 있습니다.
개선 사항 제안
-
테스트 커버리지: 새로운 브랜치에 대한 테스트가 충분히 이루어졌는지 확인하는 것이 중요합니다. 해당 브랜치에서 발생할 수 있는 모든 실패 경로를 검토하고, 그것을 테스트하는 코드를 추가하는 것을 권장합니다.
-
CI 구성 설명 추가: 추가된 브랜치가 CI/CD 파이프라인에 미치는 영향을 문서화하는 것이 좋습니다. 팀원들이 변경 사항의 목적 및 의미를 이해할 수 있도록 설명을 추가하세요.
-
브랜치 이름 일관성:
"setting/multi-module"와 같은 브랜치 이름은 특정한 컨벤션에 따라 명명되어야 합니다. 팀 내에서 통일된 브랜치 이름 규약을 따르면 혼란을 줄일 수 있습니다.
이러한 점들을 고려하여 리뷰를 진행하면 더욱 안정적이고 효율적인 CI/CD 환경을 유지할 수 있을 것입니다.
|
|
||
| test { | ||
| useJUnitPlatform() | ||
| } No newline at end of file |
There was a problem hiding this comment.
코드 패치에 대한 간단한 리뷰를 진행하겠습니다. 전체적으로 Spring Boot 프로젝트의 설정 파일로 보이며, 몇 가지 잠재적 위험 요소와 개선 사항을 제안하겠습니다.
위험 요소 및 문제점
-
중복된
repositories블록:mavenCentral()을 두 번 선언하고 있습니다. 불필요한 중복이므로 하나로 통합하세요.
-
특정 라이브러리 버전 관리:
- 외부 라이브러리의 버전을 하드코딩 사용하고 있습니다. 버전 관리를 위해
properties를 사용하는 방법을 고려하세요. 이는 유지보수성을 높일 수 있습니다.
- 외부 라이브러리의 버전을 하드코딩 사용하고 있습니다. 버전 관리를 위해
-
Deprecated API 사용:
javax.xml.bind:jaxb-api및 관련 의존성은 Java 11 이상에서 더 이상 필요하지 않습니다. 직접적인 필요성을 검토해보고 제거할 수 있다면 좋습니다.
-
Commented 코드 공유:
- 주석 처리된 라이브러리(예:
thymeleaf-extras-springsecurity6,spring-security-test)가 몇 개 있습니다. 프로젝트 내에서 필요하지 않다면 삭제하고, 필요하다면 주석 대신 추가하는 것이 좋습니다.
- 주석 처리된 라이브러리(예:
-
developmentOnly및testCompileOnly표시:- Lombok 관련 조건들이
compileOnly,annotationProcessor,testCompileOnly로 나누어져 있는데, 이는 좀 더 일관되게 관리하는 것이 좋습니다. 예를 들어, 모든 Lombok 관련 의존성을compileOnly로 통합할 수 있습니다.
- Lombok 관련 조건들이
개선 제안
-
Gradle Wrapper 사용:
- 빌드 스크립트에서 Gradle의 버전을 고정할 수 있도록 Gradle Wrapper를 사용하는 것이 좋습니다. 이를 통해 환경에 따라 발생할 수 있는 이슈를 줄일 수 있습니다.
-
디버깅 및 로깅:
- 해당 프로젝트에서 사용될 로깅 라이브러리(예: SLF4J 등)를 추가하여 디버깅과 로깅을 개선할 수 있습니다. 이는 문제 발생 시 추적을 쉽게 합니다.
-
테스트 의존성 통합:
- 테스트 관련 의존성에서
testRuntimeOnly대신testImplementation을 사용하여 의존성을 통합하고, 더 일관된 사용을 고려해보세요.
- 테스트 관련 의존성에서
-
버전 업데이트 주기:
- 외부 라이브러리는 지속적으로 업데이트가 되기 때문에, 정기적으로 의존성을 점검하고 최신 버전으로 업데이트하는 절차를 마련하는 것이 좋습니다.
위의 사항을 고려하여 코드를 수정하면 더 안전하고 유지보수하기 쉬운 프로젝트가 될 것입니다. 추가적인 질문이 있으면 언제든지 말씀해 주세요!
| public ProjectHistoryNotFoundException() { | ||
| this(PROJECT_HISTORY_NOT_FOUND_EXCEPTION.getMessage()); | ||
| } | ||
| } |
There was a problem hiding this comment.
코드 리뷰를 해드리겠습니다.
-
예외 클래스 상속 문제:
ProjectHistoryNotFoundException이BusinessException을 상속하는 것은 좋지만,BusinessException의 정의를 확인하고 실제로 적절한 예외 계층 구조를 따르는지 확인해야 합니다. 만약BusinessException이 RuntimeException의 자식이라면 문제는 없지만, 그렇지 않다면 예외 처리에 주의가 필요합니다. -
명확한 메시지 처리: 기본 생성자에서
PROJECT_HISTORY_NOT_FOUND_EXCEPTION.getMessage()를 사용하여 메시지를 전달하는 것은 좋지만, 이 메시지가 개발자 또는 사용자에게 의미 있는지를 검토하는 것이 좋습니다. 예외 메시지는 의미가 명확해야 디버깅과 문제 해결에 도움이 됩니다. -
구조의 일관성: 예외 클래스가 다른 예외 클래스와 일관된 방식으로 정의되어 있어야 합니다.
BusinessException이 다른 예외들과 어떻게 연결되는지 확인하고, 필요 시 주석을 추가하여 문서화하는 것도 도움이 됩니다. -
예외 처리: 이 예외가 발생했을 때의 처리를 문서화하는 것도 필요합니다. 호출하는 측에서 이 예외가 발생할 수 있다는 점을 고려하고, 적절한 예외 처리가 이루어질 수 있도록 명시하는 것이 좋습니다.
-
테스트 코드 작성: 이 예외 처리 클래스에 대한 단위 테스트 코드도 작성하여, 기대하는 대로 작동하는지 검증하는 것이 중요합니다. 다양한 경계 조건에 대해 테스트를 수행하는 것을 추천합니다.
이 외에도 클래스와 예외에 대한 더 많은 정보가 있다면 추가적인 논의가 가능할 것 같습니다.
| public ProjectNameDuplicatedException() { | ||
| this(PROJECT_DUPLICATED_NAME.getMessage()); | ||
| } | ||
| } |
There was a problem hiding this comment.
코드 리뷰를 해보겠습니다.
-
패키지 구조:
com.seveneleven.exception패키지에 위치하고 있는 점은 적절해 보입니다. 예외 클래스는 보통exception패키지에 두는 것이 일반적입니다. -
상수 사용:
PROJECT_DUPLICATED_NAME상수를 사용하여 예외 발생 시 코드와 메시지를 일관되게 처리하는 것은 좋은 접근입니다. 코드의 명확성을 높이는데 기여합니다. -
생성자: 두 개의 생성자를 제공하여 유연성을 높인 점도 좋습니다. 첫 번째 생성자는 사용자 정의 메시지를 받을 수 있고, 두 번째 생성자는 기본 메시지를 사용합니다.
-
예외 유형:
BusinessException을 상속받는 설계는 잘 되어있으나, 해당 클래스의 구현을 알 수 없어서BusinessException이 어떤 기능을 하는지 확인할 필요가 있습니다. 필요한 경우 적절한 예외 계층 구조를 고려해야 합니다. -
메시지 관리: 기본 메시지가 상수에서 제공되는지 확인해야 합니다. 상수가 외부에서 쉽게 수정될 수 있는 경우, 코드가 예기치 않게 동작할 위험이 있습니다. 가능한 경우 i18n(국제화) 지원을 고려하는 것이 좋습니다.
-
버그 리스크: 현재로서는 큰 버그 리스크는 없어 보입니다. 다만,
BusinessException이 어떤 종류의 예외인지 알아야 추가적인 검토나 도움을 줄 수 있을 것 같습니다. -
결론: 전체적으로 잘 작성된 코드입니다. 개선할 점으로는 문서화 및 주석을 추가하여 코드의 목적과 사용 방법을 명확히 하는 것이 좋습니다. 예외 처리에 대한 기준이나 정책이 있다면, 그에 따라 일관성 있도록 보완할 필요가 있습니다.
이와 같은 내용으로 리뷰를 마칩니다. 추가 질문이 있으시면 말씀해 주세요!
| public ProjectNotFoundException() { | ||
| this(PROJECT_NOT_FOUND.getMessage()); | ||
| } | ||
| } |
There was a problem hiding this comment.
코드 리뷰에 대한 간단한 피드백입니다.
-
클래스 구조:
ProjectNotFoundException클래스는BusinessException을 상속하고 있으며, 적절하게 메시지를 설정하고 있습니다. 기본 생성자와 매개변수 생성자가 모두 제공되어 있어 유연성이 있습니다. -
상수 사용:
PROJECT_NOT_FOUND상수를 사용하여 코드의 가독성을 높이고 있습니다. 명확한 에러 코드로서 좋은 접근입니다. -
예외 처리:
BusinessException의 사용이 적절한지 확인해야 합니다. 만약BusinessException이 모든 예외의 최상위 클래스라면, 이는 좋지만 전반적인 예외 계층 구조도 검토해야 할 수 있습니다. -
문서화: 각 생성자에 대한 주석이 없어서, 향후 이 코드를 사용하는 개발자가 이해하기 어렵게 만들 수 있습니다. 각 생성자에 대한 간단한 설명 주석을 추가하는 것이 좋습니다.
-
테스트: 이 예외가 실제로 사용되는 부분에서 적절히 테스트되고 있는지 확인해야 합니다. 예외 처리 유닛 테스트를 통해 의도한 대로 작동하는지 확인하는 것이 중요합니다.
-
예외 메시지: 기본 생성자가 상수의 메시지를 사용하고 있는데, 이 메시지가 충분히 설명적인지 확인해야 합니다. 예외가 발생할 경우 로그에 남기는 정보가 충분해야 디버깅이 용이합니다.
이 코드 패치 자체에서 직접적인 버그는 보이지 않지만, 위에서 언급한 개선 사항을 고려하는 것이 좋습니다.
| implementation 'com.h2database:h2' | ||
| testImplementation 'org.springframework.boot:spring-boot-starter-test' | ||
| //testImplementation 'org.springframework.security:spring-security-test' | ||
| testRuntimeOnly 'org.junit.platform:junit-platform-launcher' | ||
| } | ||
|
|
||
| tasks.named('test') { |
There was a problem hiding this comment.
코드 패치를 리뷰한 결과, 다음과 같은 버그 위험과 개선 사항을 발견했습니다.
-
bootJar 비활성화:
bootJar { enabled = false }설정은 Spring Boot 애플리케이션의bootJar작업을 비활성화합니다. 이는 Spring Boot 애플리케이션이 Jar로 패키징되지 않음을 의미합니다. 만약 이 프로젝트가 Spring Boot 애플리케이션이라면, bootJar를 비활성화하는 것이 의도한 행동인지 확인해야 합니다.
-
jar 설정:
jar { enabled = true }설정은 명시적으로 Jar 작업을 활성화하였습니다. 그러나 명시적인 설정 없이도 기본값이 true일 수 있으므로, 이 설정이 필요한지 다시 검토해보는 것이 좋습니다.
-
의존성 감소:
- 많은 의존성이 주석 처리되어 있습니다. 주석 처리된 의존성을 처리할 필요가 있거나 불필요한 것이라면 삭제하는 것이 좋습니다. 이로 인해 빌드 시간이 단축되고, 불필요한 의존성 관리가 줄어들 것입니다.
-
버전 관리:
- 의존성의 버전 정보를 관리하는 방법에 대해 고민해 볼 필요가 있습니다. 특정 버전에 의존할 경우, 주기적으로 해당 버전 정보를 업데이트해야 할 수 있습니다. Gradle의
properties파일을 사용하여 버전 관리의 일관성을 유지하는 것을 고려해보세요.
- 의존성의 버전 정보를 관리하는 방법에 대해 고민해 볼 필요가 있습니다. 특정 버전에 의존할 경우, 주기적으로 해당 버전 정보를 업데이트해야 할 수 있습니다. Gradle의
-
테스트 의존성:
testCompileOnly와testAnnotationProcessor와 같은 일부 테스트 관련 의존성은 Gradle의 최신 버전에서는testImplementation및testAnnotationProcessor로 교체되는 것이 바람직합니다. 또한testRuntimeOnly와 같은 의존성 관리 방법에 대한 검토가 필요합니다.
-
JUnit 의존성:
- JUnit은 주로 테스트에 사용되므로, 이를 프로젝트의 주요 의존성과 동일한 수준에서 관리하는 것이 좋습니다.
org.junit.platform:junit-platform-launcher가testRuntimeOnly의존성으로 설정된 것에 대해 검토가 필요합니다.
- JUnit은 주로 테스트에 사용되므로, 이를 프로젝트의 주요 의존성과 동일한 수준에서 관리하는 것이 좋습니다.
이 외에도 프로젝트 전체의 코드 구조나 다른 설정 파일들과의 호환성을 확인하는 것이 중요합니다. 이 리뷰를 통해 코드를 더 개선할 수 있는 기회를 가질 수 있기를 바랍니다.
| @@ -1,6 +1,6 @@ | |||
| distributionBase=GRADLE_USER_HOME | |||
| distributionPath=wrapper/dists | |||
| distributionUrl=https\://services.gradle.org/distributions/gradle-8.8-bin.zip | |||
| distributionUrl=https\://services.gradle.org/distributions/gradle-8.10-bin.zip | |||
| networkTimeout=10000 | |||
| validateDistributionUrl=true | |||
| zipStoreBase=GRADLE_USER_HOME | |||
There was a problem hiding this comment.
이 코드 패치를 검토해보면, Gradle 버전을 8.8에서 8.10으로 업데이트하는 내용입니다. 다음은 잠재적인 버그 리스크 및 개선 사항에 대한 몇 가지 제안입니다.
-
버전 호환성: Gradle 8.10의 새로운 기능이나 변경 사항이 기존 프로젝트와 호환되지 않을 수 있습니다. 업데이트 전에 릴리스 노트를 검토하여 주요 변경 사항을 확인하는 것이 좋습니다.
-
테스트: Gradle 버전을 업데이트한 후, 전체 빌드를 수행하고 테스트를 실행하여 새로운 버전에서 문제가 발생하지 않는지 확인하세요. 특히 빌드 스크립트나 플러그인에서 Gradle 버전에 의존하는 부분이 있는지 살펴야 합니다.
-
네트워크 타임아웃:
networkTimeout=10000이 설정되어 있는데, 이는 10초의 타임아웃을 의미합니다. 만약 네트워크가 불안정한 환경에서 작업하는 경우, 이 값을 늘리는 것이 좋을 수 있습니다. -
URL 검증:
validateDistributionUrl=true는 좋은 보안 관행입니다. 이 설정을 유지하는 것이 중요하며, 외부 URL로부터의 다운로드에 대한 신뢰성을 높여줍니다. -
주석 추가: 어떤 이유로 Gradle 버전을 업데이트했는지에 대한 설명을 주석으로 추가하면 코드 유지보수에 도움이 될 수 있습니다.
-
적절한 태그 관리: 버전 업데이트를 위한 태그를 Git에서 관리한다면, 어떤 변경 사항이 있었는지 쉽게 추적할 수 있으므로 이를 고려하는 것이 좋습니다.
유의 사항들을 참고하여 안전하게 업데이트 작업을 진행하시기 바랍니다.
| @@ -84,7 +86,8 @@ done | |||
| # shellcheck disable=SC2034 | |||
| APP_BASE_NAME=${0##*/} | |||
| # Discard cd standard output in case $CDPATH is set (https://github.com/gradle/gradle/issues/25036) | |||
| APP_HOME=$( cd "${APP_HOME:-./}" > /dev/null && pwd -P ) || exit | |||
| APP_HOME=$( cd -P "${APP_HOME:-./}" > /dev/null && printf '%s | |||
| ' "$PWD" ) || exit | |||
|
|
|||
| # Use the maximum available, or set MAX_FD != -1 to use that value. | |||
| MAX_FD=maximum | |||
There was a problem hiding this comment.
코드 패치에 대한 간단한 리뷰를 제공하겠습니다.
주요 변경 사항
-
SPDX 라이선스 주석 추가: 코드에 라이선스 정보를 추가함으로써 코드의 사용 조건을 명확히 하고, 라이선스 관련 법적 문제를 예방할 수 있습니다.
-
cd명령어 사용 변경:cd -P를 사용하고,pwd대신printf명령어를 사용하여 현재 디렉토리를 출력하도록 변경했습니다. 이는 더 안전한 방법입니다.
잠재적 버그 위험 및 개선 사항
-
코드 가독성:
printf '%s\n' "$PWD"부분이 어떻게 동작하는지에 대한 주석이 없다면, 가독성을 위해 주석을 추가하는 것이 좋습니다. 사용자에게 의도한 바를 명확히 하는 것은 중요합니다. -
에러 처리:
cd명령어가 실패할 경우exit를 호출하지만, 어떤 이유로 실패했는지 메시지를 출력하지 않으면 디버깅이 어려울 수 있습니다. 실패 이유에 대한 에러 메시지를 추가하면 좋겠습니다.cd -P "${APP_HOME:-./}" > /dev/null || { echo "Error changing directory"; exit 1; }
-
APP_HOME초기화:APP_HOME변수가 사용되기 전에 초기화되고, 디렉토리를 변경할 때 올바른 경로를 설정하도록 하는 것이 필요합니다. 경로가 존재하지 않거나 접근할 수 없는 경우에 대한 처리가 필요합니다. -
MAX_FD 설명: MAX_FD 변수의 목표와 용도를 주석으로 명확히 하면, 나중에 코드를 읽는 다른 사람이 이해하는 데 도움이 됩니다.
이러한 점들을 고려하면 코드의 안정성과 유지 보수성을 높일 수 있습니다. 전체적으로 좋은 방향으로 개선되고 있으니, 위의 사항들을 참고하여 수정하면 더욱 좋을 것 같습니다.
| @@ -13,6 +13,8 @@ | |||
| @rem See the License for the specific language governing permissions and | |||
| @rem limitations under the License. | |||
| @rem | |||
| @rem SPDX-License-Identifier: Apache-2.0 | |||
| @rem | |||
|
|
|||
| @if "%DEBUG%"=="" @echo off | |||
| @rem ########################################################################## | |||
There was a problem hiding this comment.
코드 패치에 대한 간단한 리뷰를 제공하겠습니다.
-
SPDX 라이센스 추가:
@rem SPDX-License-Identifier: Apache-2.0해당 주석이 추가된 것은 좋은 점입니다. 이는 프로젝트의 라이센스를 명확하게 나타내 주는 정보이며, 다른 개발자들이 법적 요건을 이해하는 데 도움이 됩니다.
-
주석의 형식:
@rem주석 처리 방식을 사용하는 것은 Windows 배치 파일에서 일반적이지만, 이 코드가 좀 더 복잡해지는 경우, 일반적인 주석 문법을 사용하는 것이 가독성에 도움이 될 수 있습니다.
-
DEBUG 환경 변수 체크:
@if "%DEBUG%"=="" @echo off조건문은 DEBUG 환경 변수에 따른 동작을 결정하는데, 이 부분에는 명시적인 에러 핸들링이 포함되어 있지 않습니다. 만약 DEBUG가 설정되어 있지 않은데, 관련한 출력을 수행할 다른 코드가 있다면 나중에 문제가 발생할 수 있습니다. 예를 들어,DEBUG가 설정된 경우에만 수행해야 할 추가적인 로직이 있다면 그에 대한 처리가 필요합니다.
-
기타 개선 사항:
- 만약 추가적인 설정이나 보조 스크립트가 존재한다면, 그에 대한 주석을 작성해 주는 것이 좋습니다. 특히 협업을 고려할 때, 다른 개발자들이 어떤 설정을 사용해야 하는지 쉽게 이해할 수 있도록 정보를 제공하는 것이 중요합니다.
-
가독성:
- 배치 스크립트가 길어질 경우, 코드의 가독성을 위해 적절하게 주석을 추가하고 공백을 활용해 주기를 권장합니다.
이 외에, 전체적인 코드의 컨텍스트를 알지 못하면 구체적인 버그를 지적하기 어려우니, 전체적인 코드 흐름과 기능적 요구사항을 고려하면서 더욱 상세히 검토하는 것이 필요합니다.
| include 'Admin' | ||
| include 'Main' | ||
| include 'Common' | ||
|
|
There was a problem hiding this comment.
코드 패치에 대한 간단한 리뷰를 진행하겠습니다.
-
정확성: 패치가
include 'Admin',include 'Main',include 'Common'을 추가하는 것으로 보입니다. 이 부분은 Gradle 프로젝트 설정에서 하위 모듈을 포함시키는 작업인데, 필요하다면 잘 작성되었습니다. -
모듈 의존성: 추가된 모듈이 다른 모듈에 의존성이 있을 경우, 해당 의존성 관리가 적절히 이루어져 있는지 확인해야 합니다. 각 모듈 간의 종속성을 제대로 설정하지 않으면 빌드 실패나 런타임 오류가 발생할 수 있습니다.
-
패키지 네이밍: 포함된 모듈의 이름이 의미를 갖고 있어야 합니다. 올바른 패키지 구조에 따라 모듈명을 결정하는 것이 좋습니다. 필요에 따라 추가 설명이나 주석을 달아주면 좋습니다.
-
빌드 파일 형식: Gradle 스크립트에서의 형식적인 오류가 없는지 다시 한번 확인해보세요. 예를 들어, 주석이나 과도한 공백이 없는지 점검이 필요합니다.
-
테스트: 모듈을 추가한 후에는 전체 시스템을 테스트하여 추가된 모듈이 기존 기능에 문제가 없는지 확인해야 합니다.
-
문서화: 새로운 모듈을 포함한 이유와 해당 모듈의 역할에 대한 documentation을 작성하는 것이 좋습니다. 이는 향후 유지보수에 큰 도움이 될 것입니다.
-
버전 관리: 만약 각 모듈에 대한 버전 관리가 필요하다면, 모듈의 버전을 명시하는 것이 좋습니다.
이러한 점들을 고려하여 코드를 관리하면 더욱 안정적이고 유지보수하기 쉬운 코드를 만들 수 있습니다.
close: #72
바로 MERGE하지 마세요! 검토 후 MERGE해야합니다!
✅ 최근 작업 주제 (하나 이상의 주제를 선택해주세요.)
🏆 구현 목표
마이페이지 API 기능 개발
📋 구현 사항 설명 (작업한 내용을 상세하게 기록합니다.)
🔍 테스트 방법 (변경 사항을 확인하기 위한 테스트 방법을 기술합니다.)