-
Notifications
You must be signed in to change notification settings - Fork 0
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Update: 파일, 링크 관련 엔티티 수정 #37
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code review by ChatGPT
FORMAT_NOT_PERMITTED_ERROR(4004, HttpStatus.BAD_REQUEST, "유효하지 않은 파일 타입입니다."), | ||
MIME_NOT_PERMITTED_ERROR(4005, HttpStatus.BAD_REQUEST, "유효하지 않은 MIME 타입입니다."), | ||
FILE_SIZE_EXCEED_ERROR(4006, HttpStatus.BAD_REQUEST, "파일 사이즈가 초과되었습니다."), | ||
|
||
|
||
// 5000번대 코드 : 서버 내부 오류 관련 | ||
INTERNAL_SERVER_ERROR(5001, HttpStatus.INTERNAL_SERVER_ERROR, "서버 내부 오류가 발생했습니다."), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
코드 패치를 검토한 결과, 몇 가지 개선점과 잠재적인 버그 위험을 발견했습니다.
-
상수의 일관성: 에러 코드는 4000번대부터 5000번대까지 차례로 나열되고 있습니다. 새로운 에러 코드들이 4001부터 시작하여 4006까지 잘 정의되어 있으나, 나중에 추가할 에러 코드가 생길 경우 중복되지 않도록 관리해야 합니다. 가능하다면 구성 요소나 범주에 따라 체계적으로 코드 번호를 할당하는 것이 좋습니다.
-
에러 메시지의 명확성: 각 에러 메시지는 사용자에게 적절한 정보를 제공하는 측면에서 잘 작성되었습니다. 그러나 각 메시지가 상황적용에 따라 다르게 해석될 여지가 있으므로, 두 번째 언어를 사용하는 사용자도 쉽게 이해할 수 있도록 조금 더 명확하게 작성할 수 있습니다.
-
Enum 추가: 에러 코드가 추가되면서 새로운 에러 상황에 대한 처리가 필요한 경우가 있을 수 있습니다. 이 경우 기존의 ErrorCode enum 클래스를 수정하지 않도록 확장 가능성을 고려하여 다른 클래스로 나누는 방법도 고려할 수 있습니다.
-
ErrorCode의 범위 관리: 추후 다른 모듈에서 에러 코드를 추가해야 할 경우, 코드 충돌을 피하기 위해 적절한 범위를 관리하는 방안이 필요합니다. 예를 들어, 파일 처리 관련 에러는 4000번대, 인증 관련 에러는 4100번대 등으로 범위를 명시하는 것이 좋습니다.
-
테스트 케이스: 새로운 에러 코드와 메시지를 도입한 후, 이와 관련된 테스트 케이스가 확보되어 있는지 확인해야 합니다. 특히 사용자 입력에 따라서 어떤 막대가 반환되는지를 점검하는 것이 중요합니다.
-
Logging: 발생할 수 있는 각 에러에 대하여 적절한 로깅이 구현되어 있는지 확인해야 합니다. 이를 통해 에러 발생 시 문제 해결에 필요한 정보를 얻을 수 있습니다.
이 외에도 코드를 구체적으로 검사하기 위해서는 해당 코드가 사용되는 컨텍스트와 연관된 메소드 및 클래스의 상세한 구조를 참고하는 것이 필요합니다.
@@ -67,7 +68,7 @@ private static void validateMimeType(String mimeType, FileCategory fileCategory) | |||
*/ | |||
private static void validateFileSize(long fileSize) { | |||
if (fileSize > MAX_FILE_SIZE) { | |||
throw new IllegalArgumentException("File size exceeds limit of 20MB."); | |||
throw new BusinessException(ErrorCode.FILE_SIZE_EXCEED_ERROR); | |||
} | |||
} | |||
} No newline at end of file |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
코드 리뷰를 진행하겠습니다.
-
예외 처리: 기존의
IllegalArgumentException
에서 비즈니스 로직에 더 적합한BusinessException
으로 변경하신 것은 좋습니다. 이렇게 하면 예외 발생 시 더 구체적인 정보를 제공할 수 있습니다. 하지만 비즈니스 예외를 사용하기 전에, 애플리케이션 전체에서 일관되게 처리할 수 있도록 예외 계층 구조에 대한 정의가 필요할 수 있습니다. -
파일 카테고리 검증:
validateFileCategory
메서드에서IllegalArgumentException
을BusinessException
으로 변경한 것은 좋은 선택입니다. 그러나 파일 카테고리가 유효하지 않을 경우 어떤 카테고리가 무효인지에 대한 메시지를 포함하는 것도 좋은 사용자 경험을 제공합니다. -
파일 확장자 및 MIME 타입 검증: 이 부분도 잘 변경되었습니다. 예외 메시지를 좀 더 정확하게 포함시키면 디버깅에 도움이 될 수 있습니다. 예를 들어, 어떤 파일 확장자가 허용되지 않는지 명시할 수 있습니다.
-
코드 중복 제거:
ObjectUtils.isEmpty(file)
와StringUtils.isBlank(fileName)
을 사용하여 null 또는 빈 상태를 체크하는 것은 코드의 가독성을 높이는 좋은 선택입니다. 이러한 유틸리티를 사용한 부분은 코드 품질을 개선합니다. -
주석: 주석이 명확하게 작성되어 있어, 각 메서드의 목적을 쉽게 이해할 수 있습니다. 그러나, 비즈니스 로직의 예외 발생 상황이나 파일 유효성 검증 로직에 대한 주석을 추가하면 더 좋을 것 같습니다.
-
파일 크기 검증: 제거되었던 크기 검증이 다시 활성화된 것은 좋습니다. 이는 큰 파일 업로드를 방지하는 데 매우 중요합니다.
-
테스트: 이 변경 사항이 적용되고 난 뒤, 파일 업로드 관련 유닛 테스트를 업데이트하고 새로운 예외 처리가 제대로 작동하는지 확인하는 것이 중요합니다.
-
메서드 응집도: 메서드들이 적절히 작은 단위로 나누어져 있어 읽기 쉽고 유지보수하기 좋습니다. 각 메서드는 하나의 책임만을 가지고 있습니다.
전반적으로, 이 패치는 코드의 가독성과 안정성을 높이며 비즈니스 로직에 맞게 잘 다듬어졌습니다. 추가적으로 위에서 언급한 사항들을 고려하여 개선할 수 있습니다.
// } | ||
// } | ||
// | ||
//} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
코드 패치에 대해 간단히 리뷰해드리겠습니다.
-
주석 처리된 코드:
- 전체 클래스가 주석 처리되어 있는 것 같습니다. 필요에 따라 주석을 제거하거나, 특정 기능이 없어질 경우 해당 코드를 완전히 삭제하는 것이 더 나을 수 있습니다. 주석 코드가 많으면 코드베이스가 혼란스러워질 수 있습니다.
-
파일 검증:
FileValidator.validateFile(file, fileCategory);
호출 후에 만약 유효성 검증에서 오류가 발생하면 예외가 발생할 텐데, 이런 상황을 처리할 코드가 없어서 예외가 발생했을 경우에도 S3에 실수로 업로드되는 것을 방지하도록 검증 로직이 강화될 필요가 있습니다.
-
파일 메타데이터 설정:
- 파일의 확장자를 추출하는 코드에서
originalFilename
의 없어질 가능성을 고려해야 합니다. 예를 들어, 파일명이 없는 경우 빈 문자열을 리턴할 수 있으므로, 이 경우에 대한 예외 처리가 필요합니다. .substring(originalFilename.lastIndexOf('.') + 1)
호출 시, 파일명이 확장자를 포함하지 않는다면lastIndexOf
가 -1을 반환하여 IndexOutOfBoundsException이 발생할 위험이 있습니다.
- 파일의 확장자를 추출하는 코드에서
-
오류 처리:
- Exception을 직접적으로 캐치하는 것보다 구체적인 예외를 처리하는 것이 향후 디버깅 및 유지보수에 유리할 수 있습니다. 예를 들어, S3와의 연결 문제, 파일 크기 문제 등으로 인해 발생할 수 있는 여러 예외를 세분화하여 처리하는 것이 좋습니다.
-
TODO 주석:
//TODO) Audit
와 같은 주석이 많이 보이는데, 이 부분이 언제까지 해결될지에 대한 일정이나 계획을 명시해줄 필요가 있습니다. 혹시라도 메타데이터에 대한 감사 기능이 놓친다면, 나중에 큰 혼란이 생길 수 있습니다.
-
성능 향상:
LocalDateTime.now()
를 여러 곳에서 호출하고 있는데, 메소드 시작 부분에서 한 번만 호출하여 변수에 저장하고 사용하는 것이 성능과 코드 일관성을 높일 수 있습니다.
이 외에도 코드의 가독성을 높이기 위해 메소드 내의 주석들을 보강하거나 더 구체화하는 것도 좋은 접근이 될 수 있습니다. 특히, 사람들이 코드를 읽을 때 쉽게 이해할 수 있도록 도와주는 것이 중요합니다.
// } | ||
// } | ||
//} No newline at end of file |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
코드 리뷰를 요청하신 내용에 대해 간단히 설명드리겠습니다.
-
주석 처리된 코드: 전체 클래스가 주석 처리되어 있습니다. 주석 처리된 코드를 제거하고 필요한 부분만 남겼는지 확인하는 것이 좋습니다.
-
예외 처리:
uploadFile
메소드에서 IOException 발생 시 RuntimeException으로 감싸서 던지고 있습니다. 어떤 IOException인지에 대한 정보가 명확하지 않을 수 있으므로, 오류 메시지에 원래 IOException의 내용을 포함시키는 것도 좋겠습니다. -
파일 확장자 처리:
generateUniqueFileName
메소드에서 파일 이름에서 확장자를 가져올 때, 파일 이름에 확장자가 없을 경우에 대한 예외 처리가 필요합니다. 이를 방지하기 위해lastIndexOf
결과를 체크하여 예외적인 상황을 처리해야 합니다.int lastIndex = originalFileName.lastIndexOf("."); if (lastIndex == -1) { throw new IllegalArgumentException("파일에 유효한 확장자가 없습니다: " + originalFileName); } String fileExtension = originalFileName.substring(lastIndex);
-
메타데이터 설정 시 null 체크:
uploadFile
메소드에서file.getContentType()
이 null인 경우도 있을 수 있으므로, 이를 null 체크 후 적절한 기본값(예:"application/octet-stream"
)을 설정하는 것이 좋습니다. -
파일 다운로드 시
inputStream
처리:downloadFile
메소드에서 S3 객체가 null이 아닐 때만inputStream
을 닫는 방식보다, try-with-resources 구문을 사용해서inputStream
의 자원 관리를 보장하는 것이 좋습니다. -
버킷 이름을 외부에서 관리: 버킷 이름을 application.properties 파일에서 관리하는 것은 좋지만, 환경 변수를 통해 동적으로 설정할 수 있는 방법을 고려해 볼 수도 있습니다.
-
메소드 설명 주석 보완: 각 메소드 설명 주석을 작성할 때, 예외 발생 가능성과 그에 대한 설명도 추가하는 것이 좋습니다. 이처럼 명확한 문서화는 코드를 이해하는 데 도움이 됩니다.
이러한 사항들을 고려하여 코드를 수정하면 가독성과 안정성을 높일 수 있을 것입니다. 각 기능이 수행되는 상황을 예외 처리로 잘 다루는 것이 중요합니다.
FILE_METADATA_HISTORY_DELETE("파일 삭제 이력"); | ||
|
||
private final String description; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이 코드 패치에 대한 간단한 코드 리뷰를 진행하겠습니다.
리뷰 내용
-
전반적인 구조:
FileHistoryType
열거형(enum)은 파일 메타데이터의 등록 및 삭제 이력을 정의하는 역할을 잘 수행하고 있습니다.
-
Lombok 사용:
@Getter
와@RequiredArgsConstructor
어노테이션을 통해 getter 메서드와 생성자를 자동으로 생성하는 방식은 코드의 간결성을 높이는 좋은 선택입니다.
-
상수 네이밍:
- 상수 이름인
FILE_METADATA_HISTORY_REGISTER
와FILE_METADATA_HISTORY_DELETE
는 의미가 명확하여 가독성이 좋습니다.
- 상수 이름인
개선 사항
-
주석 추가:
- 코드에 대한 설명이나 각 열거형 상수에 대한 설명을 주석으로 추가하면 다른 개발자가 코드를 이해하는 데 도움이 될 수 있습니다.
-
사용 예시:
- 이 열거형이 사용될 예제를 코드에 포함시키면, 유지보수 시나리오나 다른 개발자를 위한 문서화에 유용할 수 있습니다.
-
다국어 처리:
- 현재 설명을 한국어로만 작성하였는데, 만약 국제화(i18n)를 고려해야 한다면, 다국어 지원 방식에 대해 고민할 필요가 있습니다.
-
예외 처리:
- 사용될 때 잘못된 값이 전달될 경우의 예외처리에 대한 고려가 필요할 수 있습니다. 예를 들어, 사용자 정의 메서드를 추가하여 특정 동작 시 유효성을 검사할 수 있습니다.
버그 위험
- 현재로서는 명백한 버그 위험 요소는 보이지 않지만, 열거형이 사용되는 컨텍스트에서 Enum 상수의 잘못된 사용 가능성을 항상 염두에 두어야 합니다.
이러한 사항들을 고려하여 코드를 개선해 나가면 더욱 견고하고 유지보수하기 쉬운 코드가 될 것입니다.
// } | ||
// | ||
// | ||
//} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
코드 패치를 검토해본 결과, 주석 처리된 파일 메타데이터 DTO 클래스의 구조와 기능에 대해 몇 가지 의견을 제시합니다.
-
유효성 검사 미구현: TODO 주석이 있는 것으로 보아, 유효성 검사 기능이 아직 구현되지 않았습니다. 유효성 검사는 필드 값이 올바른지 검증하는 중요한 작업이므로, 이 부분을 반드시 추가해야 합니다.
-
writtenAt
필드 값 설정:writtenAt
필드에 현재 시간을 설정하는 로직이 있습니다. 엔티티에 따라 이 값이 변경될 수 있으므로, DTO를 생성할 때 외부에서 전달받은 값으로 설정하거나, 메타데이터 엔티티의 날짜를 사용하는 것이 더 낫습니다. 현재 시간을 자동으로 할당하는 대신, 매개변수로 받아서 설정하도록 변경할 수 있습니다. -
이너 클래스 및 메서드: DTO와 엔티티 변환 메서드는 DTO 클래스 안에 위치하는 것이 일반적입니다. 하지만 코드가 주석 처리되어 있어, 전체 구조나 다른 부분과의 관계를 확인할 수 없으므로, 전체 프로젝트 구조와 일관성을 고려하여 위치를 검토하는 것이 좋습니다.
-
패키지 주석 처리: 패키지 선언 부분을 주석 처리하는 것은 코드 가독성을 떨어뜨립니다. 필요하다면, 주석 처리보다는 아예 해당 파일이 사용되지 않도록 삭제하거나, 버전 관리 시스템을 통해 해당 코드의 변경 사항을 확인할 수 있는 방법을 사용하는 것이 좋습니다.
-
다중 인스턴스 문제: DTO의 생성 메서드인
toDto
가 정적 메서드로 되어 있는데, 이는 다양한 변환 로직을 갖는 경우 관리에 어려움을 초래할 수 있습니다. 변환 로직이 복잡해질 경우, 책임이 분리된 팩토리 클래스를 도입하는 방법도 고려해볼 수 있습니다. -
필드 주석 번역: 주석에 사용된 한국어는 상황에 따라 적절하지만, 국제화를 염두에 두고 영어 주석을 추가하면 다른 팀원들과의 협업에서 도움이 될 것입니다.
이 외에 코드 스타일 또는 형식에 대한 일관성을 유지하는 것도 중요합니다. 코드가 주석 처리가 되어 있어 현재 작동하지 않지만, 위의 제안들을 바탕으로 파일 메타데이터 DTO를 개선하면 좋을 것 같습니다.
fileMetadata.filePath = filePath; | ||
|
||
return fileMetadata; | ||
} | ||
} No newline at end of file |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이 코드 패치에 대한 간단한 코드 리뷰를 제공합니다.
코드 리뷰
-
주석 추가: 새로운 상수들의 정의에 관련된 주석이 포함되어 있어서 가독성 향상에 기여합니다. 그러나 각 변수의 용도와 필요성을 좀 더 명확히 설명하는 주석을 추가하면 좋겠습니다.
-
부적절한 Access Level:
createdBy
와createdAt
필드는private
접근 제어자로 유지되고 있지만, 이들이 사용될 곳에서 접근할 필요가 있을 수 있습니다. 일반적으로 JPA 엔티티에서 Auditing 관련 필드는protected
나public
으로 설정하는 것이 좋습니다. -
제한 없는 필드:
contentType
,fileFormat
,filePath
등의 필드는 유효성 검사 없이 직접 저장됩니다. 파일의 메타데이터는 특정 형식이나 길이 제한이 있을 수 있음을 고려하여 검증 로직을 추가하는 것이 좋습니다. -
매직 넘버 문제: 각 필드의 최대 길이와 정밀도를 상수로 정의한 것은 좋은 접근입니다. 앞으로 추가적인 제한이 필요하다는 점을 반영하여 이 부분은 잘 구성되었습니다.
-
DTO 및 검증:
registerFile
메서드는 DTO를 사용하여 전달된 데이터를 검증하거나 일관성을 확인하는 추가적인 로직이 필요할 수 있습니다. 간단한 팩토리 메서드지만, 입력값이 유효한지 체크하는 로직이 없어서 RuntimeException이 발생할 수 있습니다. -
메서드 접근 제어자:
registerFile
메서드는public static
으로 정의되어 있습니다. 이는 외부에서 호출할 수 있지만, 클래스 내부에서만 사용된다면private static
또는protected static
으로 변경하는 것이 좋습니다. -
동일한 정보:
fileSize
필드의 주석에는 "KB 단위"라고 명시되어 있으나, 만약에 다른 단위로 변경될 가능성이 있다면, 주석이 필요하지 않을 수 있습니다.
결론
이 코드 패치는 전체적으로 가독성이 좋고, 적절한 구조를 가지고 있습니다. 그러나 몇 가지 동적 유효성 검사 추가와 접근 제어자를 재검토할 필요가 있습니다. 예외 처리 및 검증 로직에 대한 추가적인 검토가 필요할 수 있습니다.
fileDisplayTitle, fileTitle, writerEmail, writerName, | ||
writerAuthority, writtenAt, contentType, fileFormat, fileSize, filePath); | ||
} | ||
|
||
} No newline at end of file |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이 코드 패치는 FileMetadataHistory
엔티티에 대한 여러 개선 사항과 변경 사항을 포함하고 있습니다. 아래는 코드 리뷰에 대한 몇 가지 피드백입니다.
개선 사항
-
매직 넘버 상수화:
MAX_*
상수를 정의하여 코드 가독성이 향상되었습니다. 이는 유지보수에 큰 도움이 됩니다. -
ENUM 사용:
historyType
과fileCategory
를String
에서 ENUM으로 변경한 것은 데이터 무결성을 높이고 실수를 줄이는 데 기여합니다. -
공통 생성 메서드 사용:
createHistory
메서드를 통해FileMetadataHistory
객체를 생성하는 로직을 통합했습니다. 이는 코드 중복을 줄이고, 한 곳에서 객체 생성 로직을 관리할 수 있게 합니다. -
세미콜론 누락: 주석 뒤의 빈 줄(
//
)에서 세미콜론이 누락되었습니다. 이는 문제가 되지 않지만, 일관성을 위해 주석을 붙이는 위치를 고려해보면 좋습니다. -
컬럼 주석: 각 필드에 대한 설명이 잘 추가되어 있어 이해하기 쉬웠습니다. 하지만 주석은 코드와 함께 업데이트되기 때문에, 나중에 수정 시 주석도 함께 관리해야 합니다.
-
로그인 사용자 정보:
createdBy
필드의 주석을 개선하여, 이 필드의 역할이 명확히 드러나도록 하였습니다. 하지만createdAt
과writtenAt
필드의 생성 주체에 대한 설명도 추가하는 것이 좋겠습니다.
버그 리스크
-
createdBy
와 같은 필드의 초기값: 엔티티가 DB에서 생성될 때createdBy
필드를 수동으로 설정하지 않으면 null이 될 수 있습니다. 적절한 기본값을 설정하거나, 생성 시 로그인이 필요한 경우 이를 처리하는 로직을 추가하는 것이 좋습니다. -
필드 유효성 검사: 값이 저장되기 전에 각 필드에 대한 유효성 검사를 수행하는 로직이 보이지 않습니다. 예를 들어, 이메일 형식 확인 및 문자열 길이 체크 등을 추가하여 데이터의 정합성을 보장하는 것이 좋습니다.
-
Double 사용 시 주의:
fileSize
를Double
로 설정했는데, 이 값의 정확성을 유지하기 위해서는 메모리 및 산술 오차를 주의해야 합니다. 필요에 따라BigDecimal
을 사용하는 것도 고려해볼 수 있습니다. -
동시성 문제: 여러 스레드가 동시에
FileMetadataHistory
를 수정할 수 있다는 점에서, 동시에 접근 시 동기화를 고려해야 합니다.
이러한 피드백을 바탕으로 코드의 안정성과 가독성을 더욱 높일 수 있을 것입니다.
linkData.link = link; | ||
|
||
return linkData; | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
코드 패치에 대한 간단한 코드 검토를 진행해보겠습니다.
버그 리스크
-
유효성 검사 부족:
registerLink
메서드에서link
파라미터에 대한 유효성 검사가 없습니다. 링크의 길이가MAX_LINK_LENGTH
를 초과하지 않는지 체크해야 합니다. 예를 들어, 다음과 같이 검사를 추가하는 것이 좋습니다.if (link == null || link.length() > MAX_LINK_LENGTH) { throw new IllegalArgumentException("Invalid link"); }
-
데이터 무결성:
referenceId
와 같은 필드도 유효성 검사를 추가하면 좋습니다. 예를 들어,null
값이 허용되지 않도록 체크할 수 있습니다.
개선 사항
-
Immutable 객체:
Link
클래스에서 사용 중인@Builder
어노테이션이 삭제되었습니다. 불변 객체를 선호하는 경우@Builder
를 유지하고,registerLink
메서드에서 생성된 객체를 반환할 때 빌더 방식을 사용하는 방법을 고려할 수 있습니다. 이렇게 하면 나중에 객체의 불변성이 보장됩니다. -
로그 추가: 메서드나 중요한 필드가 변경되는 경우 로깅을 추가하면 디버깅과 유지보수에 도움이 됩니다.
-
주석 개선: 주석은 좋지만, 조금 더 상세하게 작성되면 코드의 가독성을 높일 수 있습니다. 예를 들어, 각 필드에 대해 어떤 케이스에서 사용되는지를 설명하는 것이 좋습니다.
-
리턴 타입의 명세화:
registerLink
메서드와 같은 경우에는 리턴 타입을 명확히 하고, 반환된 객체의 사용 목적에 대한 설명을 추가하는 것이 좋겠습니다.
기타 사항
@Table
어노테이션에서name
속성을link
에서 다른 이름으로 변경하는 것도 고려해볼 수 있습니다. 일반적으로 테이블 이름은 명확하고 일관되게 지정하는 것이 좋습니다.- 코딩 스타일에 일관성을 유지하는 것도 중요합니다. 코드의 포맷팅이나 주석 작성 방식에서 통일성을 지키는 것이 좋습니다.
이와 같은 사항들을 고려하여 코드를 개선하면 더 안전하고 가독성이 높은 코드를 작성할 수 있습니다. 추가적인 질문이나 수정 사항이 필요하시면 알려주세요!
linkCategory, referenceId, referenceIdentifier, | ||
writerEmail, writerName, writerAuthority, writtenAt, link); | ||
} | ||
|
||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
코드 패치에 대한 간단한 리뷰를 하겠습니다.
개선점 및 리스크
-
주석 및 코드 가독성:
- 각 필드에 대한 주석이 잘 작성되어 있어 코드의 가독성을 높이고 이해하기 쉽게 만듭니다. 그러나 불필요한 주석이 몇 군데 있는데, 예를 들어, "시스템 컬럼"이라는 주석은 코드에서 명확히 드러나는 부분이므로 제거해도 좋을 것 같습니다.
-
Magic Number:
- MAX_LENGTH 상수를 도입한 것은 좋은 접근입니다. 그러나 이러한 상수들이 클래스와 같은 패키지 내에서 하드코딩된 문자열로 정의된 것이 아닌, 별도의 enum이나 설정 파일에서 관리되면 더욱 유연한 코드가 될 수 있습니다.
-
객체 생성 메서드:
createHistory
메서드를 이용한 객체 생성을 확실히 코드의 중복을 줄이고 있습니다. 하지만, 이 메서드가 모든 필드를 받도록 되어 있어 파라미터의 수가 많아질 경우 사용하기 어렵습니다. 빌더 패턴을 도입하면 사용자에게 더 나은 유연성을 제공할 수 있을 것 같습니다.
-
정합성 검증:
- 필드에 대한 검증 로직이 없다면, 빈 필드나 잘못된 데이터가 생성될 위험이 있습니다. 예를 들어,
writerEmail
필드에 이메일 형식 검증을 추가하거나,referenceIdentifier
의 길이를 체크하는 방법이 좋습니다. 이러한 검증은 생성 시 또는 setter에서 수행할 수 있습니다.
- 필드에 대한 검증 로직이 없다면, 빈 필드나 잘못된 데이터가 생성될 위험이 있습니다. 예를 들어,
-
Immutable한 객체 선호:
LinkHistory
객체의 필드들이 mutable하게 설정되어 있어, 객체 생성 후 변화할 가능성이 있습니다. 불변성을 선호한다면 필드 접근자를 private으로 설정하고 생성자를 통해 초기화한 후, setter 메서드를 제공하지 않는 것이 좋습니다.
-
하드코딩된 컬럼 이름:
- 데이터베이스 컬럼 이름을 코드 내 하드코딩했는데, 데이터베이스 변경이 있을 경우 유지보수가 어려울 수 있습니다. Enum 또는 상수를 사용하는 것도 한 방법입니다.
-
예외 처리:
- 데이터베이스와 상호작용하는 부분에서 예외 처리가 이루어지지 않으면, 적절한 오류 처리가 필요할 수 있습니다. 예외 발생 시에는 로그를 남기거나 사용자에게 피드백을 줄 수 있는 방안도 고려해야 합니다.
결론적으로, 코드 개선 여지가 있고 리팩토링을 통해 가독성과 유지 보수성을 높일 수 있는 부분이 있습니다. 추가적인 검증 로직 및 불변성 유지가 중요할 것 같습니다.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 👍
src/main/java/com/seveneleven/devlens/global/util/file/FileValidator.java
Show resolved
Hide resolved
src/main/java/com/seveneleven/devlens/global/util/file/constant/LinkHistoryType.java
Show resolved
Hide resolved
src/main/java/com/seveneleven/devlens/global/util/file/entity/LinkHistory.java
Outdated
Show resolved
Hide resolved
src/main/java/com/seveneleven/devlens/global/util/file/entity/FileMetadataHistory.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code review by ChatGPT
@RequiredArgsConstructor | ||
public enum FileHistoryType { | ||
REGISTER("파일 등록 이력"), | ||
DELETE("파일 삭제 이력"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
코드 패치에 대한 간단한 리뷰를 진행하겠습니다.
-
명명 규칙: Enum 값의 이름을
FILE_METADATA_HISTORY_REGISTER
와FILE_METADATA_HISTORY_DELETE
에서REGISTER
와DELETE
로 변경한 점은 긍정적입니다. 더 간결하고 명확한 표현입니다. 다만, 이러한 변경은 코드의 가독성과 유지보수성에 기여할 수 있습니다. 만약 이 Enum이 더 큰 코드베이스에서 사용된다면, 명확한 이름(예:FILE_REGISTER
및FILE_DELETE
)이 더 도움이 될 수 있습니다. -
주석 및 문서화 부족: Enum의 용도에 대한 주석이 없으므로, 다른 개발자가 이 코드를 읽을 때 Enum의 역할을 파악하는 데 어려움이 있을 수 있습니다. Enum의 목적이나 사용 예시를 주석으로 추가하는 것이 좋습니다.
-
불변성 유지:
description
필드는 final로 선언되어 변경할 수 없으므로, 인스턴스 생성 후 안전하게 사용할 수 있습니다. 이는 좋은 패턴입니다. -
향후 확장성: 현재 두 가지 유형만 있지만, 추가적인 이력이 필요해질 경우 Enum에 쉽게 추가할 수 있습니다. 다만, Enum이 너무 커지면 오히려 관리가 어려워질 수 있으니, 필요에 따라 다른 클래스로 분리할 수도 있습니다.
-
국제화: 문자열이 한국어로 하드코딩되어 있어서 다국어 환경에서 사용하기에는 불편할 수 있습니다. 필요한 경우 국제화(i18n) 방식을 도입해 다국어 지원을 고려할 수 있습니다.
요약하자면, 코드 변경은 긍정적인 방향이지만, 문서화 및 국제화에 대해 고려할 필요가 있습니다.
@RequiredArgsConstructor | ||
public enum LinkHistoryType { | ||
REGISTER("링크 등록 이력"), | ||
DELETE("링크 삭제 이력"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
코드 패치에 대한 간단한 리뷰를 진행하겠습니다.
코드 변경 사항:
LINK_HISTORY_REGISTER
와LINK_HISTORY_DELETE
라는 이름을 각각REGISTER
와DELETE
로 변경했습니다.
코드 리뷰:
-
변수 이름 변경:
- enum 상수의 이름이 더욱 간결해졌습니다. 이는 가독성을 높이는 데 긍정적인 변화입니다. 그러나, 기존의 네이밍 스타일 (대문자 + 언더스코어)이 일관되지 않게 될 수 있으므로, 팀의 코드 스타일 가이드에 따라 결정해야 할 사항입니다.
-
기술적 위험:
- 이름이 변경됨에 따라, 이 enum을 참조하고 있는 코드에서 기존 이름을 사용하는 부분이 있으면 컴파일 오류가 발생할 수 있습니다. 이러한 점에서, 충분한 테스트 및 코드 검토가 필요합니다.
- 특정한 이름을 삭제함으로써, 만약 사용자가 해당 Enum을 참조하는 코드의 직관성을 해칠 우려가 있으므로, 코드베이스 전반에서의 참조를 확인하는 과정이 필요합니다.
개선 제안:
-
주석 추가: Each enum constant is now more general. If there are any specific contexts in which those constants are used, adding comments could help future developers understand their purpose better.
-
문서화: Enum 타입이 사용되는 위치나 그 의미에 대한 문서화가 필요할 수 있습니다. 예를 들어, 이 enum이 어딘가에 사용되는 시나리오(예: 로그 기록, 사용자 행동 추적 등)에 대한 설명을 추가하면 좋습니다.
-
테스트 케이스: 변경된 부분에 대한 테스트 케이스가 충분히 마련되어 있는지 확인하는 것도 중요합니다. 기존 사용 코드와의 호환성을 유지하기 위해 기존 기능에 대한 회귀 테스트를 진행하는 것이 좋습니다.
이상의 리뷰를 참고하여, 코드 변경 사항의 영향을 최소화하고 가독성을 높일 수 있는 방향으로 개선해 나갈 수 있기를 바랍니다.
@Column(name = "reference_identifier", length = 300, nullable = false) | ||
private String referenceIdentifier; // 참조 구분자 | ||
//회사로고 - 회사명, 계정프로필 - 계정이메일, 프로젝트 이미지 - 프로젝트명, 게시물 파일 - 게시물 제목 | ||
// 회사로고 - 회사명, 계정프로필 - 계정이메일, 프로젝트 이미지 - 프로젝트명, 게시물 파일 - 게시물 제목 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
코드 패치를 검토한 결과는 다음과 같습니다:
-
버그 위험:
writerAuthority
필드를 문자열에서Role
열거형으로 변경하는 것은 타입 안전성을 높이지만, 이 변경으로 인해 기존 코드와의 호환성 문제가 발생할 수 있습니다. 기존에 문자열로 작성된 데이터가Role
로 변환되지 않으면 에러가 발생할 수 있습니다. 또한, Role 상수가 변경될 경우 관련된 모든 부분을 업데이트해야 합니다.
-
타입 변경에 따른 영향:
writerAuthority
의 타입이 문자열에서 열거형으로 변경됨에 따라 해당 필드를 사용하는 모든 메서드가 이 변경에 영향을 받을 수 있습니다. 모든 메서드와 호출부의 일관성을 확인해야 합니다.
-
명명 규칙:
createHistory
메서드 호출 시FileHistoryType
상수명을 더 명확하게 작성하여 코드의 가독성을 높일 수 있습니다. 예를 들어,FileHistoryType.FILE_METADATA_HISTORY_REGISTER
를FileHistoryType.REGISTER
로 변경한 점은 코드의 가독성을 높이는 방향으로 좋습니다.
-
주석 개선:
- 코드 변경 사항에 대해 주석을 추가하면 후에 이 코드를 유지보수할 때 도움이 될 수 있습니다. 특히 권한 정보를 어떻게 처리하는지에 대한 설명이 필요할 수 있습니다.
-
유닛 테스트:
- 새로운 타입(
Role
)이 도입됨에 따라 기존의 유닛 테스트가 영향을 받을 수 있습니다. 이 코드 변경 이후에 관련된 유닛 테스트를 확인하고 필요 시 업데이트하여 모든 경로를 잘 커버하는지 확인하는 것이 중요합니다.
- 새로운 타입(
-
기타 개선 제안:
- Enum 타입을 사용하는 것에 대해 이득이 많이 있을 수 있으므로, 관련 데이터베이스 스키마가 이와 적합하게 설정되어 있는지도 체크해보세요. 예를 들어, DB에 적절한
Enum
매핑이 설정되어 있는지 확인하는 것이 좋습니다.
- Enum 타입을 사용하는 것에 대해 이득이 많이 있을 수 있으므로, 관련 데이터베이스 스키마가 이와 적합하게 설정되어 있는지도 체크해보세요. 예를 들어, DB에 적절한
결론적으로, 이번 코드 패치로 인해 타입 안전성은 높아지나, 기존 호환성과 관련된 부분에 대해 각별한 주의가 필요하며, 관련된 부분을 충분히 검토하고 테스트해야 합니다.
private String writerName; // 등록자 이름 | ||
|
||
@Column(name = "writer_authority", length = 50, nullable = false) | ||
private String writerAuthority; // 등록자 권한 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
코드 리뷰를 진행하겠습니다.
주요 변경사항
writerAuthority
타입을String
에서Role
열거형으로 변경하였습니다.MAX_WRITER_AUTHORITY_LENGTH
을 200에서 50으로 축소하고,MAX_LINK_LENGTH
는 200에서 1000으로 늘렸습니다.LinkHistoryType
상수의 이름을 보다 간결하게 변경하였습니다. (예:LINK_HISTORY_REGISTER
에서REGISTER
로)
버그 위험 및 개선 제안
-
타입 변경 확인:
writerAuthority
를String
에서Role
로 변경한 부분은 데이터베이스에 저장되는 방식에 영향을 미칩니다. 이 변경이 기존 데이터와의 호환성을 보장하는지 확인해야 합니다. 데이터베이스 스키마의 수정이 필요할 수 있습니다.
-
Magic Number 수정:
MAX_WRITER_AUTHORITY_LENGTH
가 50으로 변경되었으나, 이 값이 충분한지 검토가 필요합니다. 권한의 길이가 50자로 제한될 경우, 향후 개발에 제약이 될 수 있습니다.
-
Enum 사용의 장점:
Role
을 Enum으로 사용하는 것은 안전성을 높이고 코드 가독성을 개선하지만, 이를 저장할 때 데이터베이스와의 일치성을 위해@Enumerated(EnumType.STRING)
을 사용한 점은 적절합니다. 다만, 각 권한의 변경사항이 있을 경우 열거형을 수정해야 하므로, 이 점을 문서화하여 유지보수 시 주의를 기울이도록 해야 합니다.
-
상수 명명 개선:
- 상수가 실제 목적을 명확하게 전달하고 있는지 확인하고, 필요에 따라 식별자 이름을 재검토하는 것이 좋습니다. 예를 들어,
MAX_LINK_LENGTH
는 링크의 최대 길이를 나타내고 있지만, 더 구체적인 설명을 추가할 수 있습니다.
- 상수가 실제 목적을 명확하게 전달하고 있는지 확인하고, 필요에 따라 식별자 이름을 재검토하는 것이 좋습니다. 예를 들어,
-
메서드 구조 개선:
createHistory()
메서드의 매개변수가 많아지는 경향이 있습니다. 이를LinkHistory
객체로 감싸는 방법도 고려해 볼 수 있습니다. 이러면 코드가 더 깔끔해질 수 있습니다.
결론
전반적으로, 코드 변경이 구조적이고 가독성을 높이는 방향으로 진행되었습니다. 그러나 데이터베이스와의 호환성과 관련된 몇 가지 점을 고려해야 합니다. 추가적인 검토를 통해 이러한 사항들이 반영되어야 할 것입니다.
바로 MERGE하지 마세요! 검토 후 MERGE해야합니다!
✅ 최근 작업 주제 (하나 이상의 주제를 선택해주세요.)
🏆 구현 목표
파일, 링크 관련 엔티티 수정
📋 구현 사항 설명 (작업한 내용을 상세하게 기록합니다.)
🔍 테스트 방법 (변경 사항을 확인하기 위한 테스트 방법을 기술합니다.)
MySQL workbench 확인
🛠️ 쓰이는 모듈
💬 기타 질문 및 특이 사항