Feature: 회사 로고 업로드 기능 추가 #45
Conversation
kdh10806
requested review from
Eseas,
taekminKwon,
K-KY,
HiSeungmin and
jeongns2611
as code owners
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| } | ||
| public void removeRepresentativeImage(){ | ||
| this.representativeImageExists = YN.N; | ||
| } | ||
| } No newline at end of file |
There was a problem hiding this comment.
코드 패치를 검토한 결과, 다음과 같은 몇 가지 문제점과 개선 제안을 제시할 수 있습니다.
-
접근 제어:
addRepresentativeImage()와removeRepresentativeImage()메서드가public으로 설정되어 있습니다. 이 메서드들이 클래스 외부에서 접근될 필요가 없다면,private이나protected로 변경하여 캡슐화를 강화할 수 있습니다. -
매직 문자열:
YN.Y와YN.N이 무슨 의미인지 명확하지 않습니다. 매직 문자열을 사용하기보다는, 더 설명적인 상수명이나enum을 사용하는 것이 좋습니다. 코드의 가독성이 높아질 것입니다. -
예외 처리: 대표 이미지가 이미 존재하지 않는 경우에
removeRepresentativeImage()메서드를 호출할 때, 예외를 처리하거나 해당 상태를 확인하는 로직을 추가하는 것이 좋습니다. 예를 들어,representativeImageExists가 이미YN.N인 경우 아무런 동작을 하지 않도록 하거나, 로그를 남기는 등의 처리가 필요해 보입니다. -
Null 체크: 메서드 내에서
representativeImageExists가 null일 가능성을 항상 염두에 두고 체크하도록 구현하면 좋습니다. 현재 코드에는 이와 관련된 체크가 없습니다. -
주석 추가: 주석이 있지만, 메서드의 실제 동작에 대한 설명이 부족합니다. 각 메서드의 목적과 사용법에 대한 더 구체적인 설명을 추가하면 유지보수가 용이해집니다.
이러한 점들을 개선하면 코드의 안정성과 가독성을 높일 수 있습니다.
|
|
||
| public static <T> APIResponse<T> create(int code, String message, T data) { | ||
| return new APIResponse<>(code, message, data); | ||
| } | ||
| } No newline at end of file |
There was a problem hiding this comment.
코드 리뷰에 대한 피드백은 다음과 같습니다.
-
메서드 삭제:
create(int code, String message, T data)메서드가 삭제되었습니다. 이 메서드는 다양한 HTTP 상태 코드와 메시지를 제공할 수 있는 유용한 기능인데, 이 기능을 제거하면서 메서드의 유연성이 줄어들었습니다. 만약 이 메서드가 실제로 사용되지 않는다면 삭제하는 것이 괜찮겠지만, 다른 부분에서 활용됐다면 신중해야 합니다. -
새 줄 부족: 파일 끝에 새 줄이 없다는 경고가 있습니다. 일부 프로그래밍 언어에서는 파일의 끝에 새 줄을 추가하는 것이 좋은 관습으로 여겨지므로, 이를 추가하는 것이 좋습니다.
-
주석 추가: 코드의 의도나 특정 부분의 기능을 잘 설명하는 주석이 부족합니다. 코드의 가독성을 높이기 위해 각 메서드에 대한 간단한 설명을 추가하는 것이 좋습니다.
-
예외 처리: 메서드에서 데이터의 유효성 검사를 고려하지 않았습니다.
create(T data)메서드에서 유효하지 않은 데이터가 들어올 경우 예외를 발생시키는 것이 중요할 수 있습니다. 적절한 예외 처리를 추가하는 것을 고려해 보세요. -
리팩토링 고려:
create메서드가 두 개 만들어져 있는데, 만약 공통된 로직이 있다면 이를 하나의 메서드로 통합하여 코드 중복을 줄일 수 있습니다. 다만, 이 경우에는 코드의 가독성이 저하되지 않도록 주의해야 합니다.
전체적으로 코드가 깔끔하고 직관적이지만, 몇 가지 기능적 또는 구조적인 개선점을 고려할 수 있습니다.
| //5. 반환 | ||
| return uploadResponse; | ||
| } | ||
| } |
There was a problem hiding this comment.
코드 리뷰를 시작하겠습니다.
-
주석 및 문서화:
- 메서드에 대한 설명이 잘 되어 있으나, TODO 주석에는 보다 구체적인 구현 방안이나 고려할 사항을 포함하면 좋을 것 같습니다. 예를 들어, admin과 super 권한을 어떻게 판별할지에 대한 명확한 계획을 기록해 두면 좋습니다.
-
예외 처리:
uploadLogoImage메서드는Exception을 던진다고 선언되어 있습니다. 구체적인 예외(예:IOException,BusinessException등)를 명시하는 것이 코드의 가독성과 유지 관리에 더 도움이 될 것 같습니다.- 업로드 실패 시에 대한 예외 처리나 사용자 피드백을 추가하는 것도 중요합니다. 현재는 파일 업로드 실패에 대한 방어 코드가 없는 것 같습니다.
-
트랜잭션 관리:
- 메서드가 트랜잭션으로 설정되어 있음은 Good. 하지만,
fileService.uploadFile호출에 실패할 경우에 DB의 상태가 오염될 수 있는지 여부를 점검해야 합니다. 특히, 회사의 대표 이미지 유무를 변경하는 부분이 문제가 될 수 있습니다.
- 메서드가 트랜잭션으로 설정되어 있음은 Good. 하지만,
-
메서드 설계:
addRepresentativeImage메서드가 내부적으로 무엇을 하는지를 명확히 알 수 없어, 해당 메서드의 구현과 목적을 다른 개발자도 이해할 수 있게 문서화 해두는 것이 필요합니다.
-
인수 검증:
file,companyId,uploaderId등의 파라미터가 유효한지 검증하는 로직이 필요합니다. 예를 들어,file이 null이 아닌지,companyId가 음수가 아닌지 등을 체크할 수 있습니다.
-
성능 고려:
- 업로드되는 파일의 크기나 형식에 따라 성능에 영향을 줄 수 있으므로, 이러한 부분에 대한 검증을 추가하는 것도 필요합니다.
-
반환 데이터의 일관성:
- APIResponse의 구조가 어떻게 되어 있는지를 알고 있지 않지만, 메서드 반환 시에 어떤 정보를 담고 있는지 명확하게 정의해두면 좋습니다. 가능하다면 성공/실패 여부와 함께 추가 정보를 반환하도록 설계할 수도 있습니다.
이상으로 코드 리뷰를 마치겠습니다. 코드를 수정하고 테스트하는 과정에서 위에서 언급한 사항들을 고려해 보시기 바랍니다.
| @@ -69,7 +68,6 @@ public APIResponse uploadFile(MultipartFile file, Long uploaderId, String fileCa | |||
| } catch (Exception e){ | |||
| //저장 실패시 S3에서 삭제 | |||
| s3ClientService.deleteFile(s3Key); | |||
|
|
|||
| throw new BusinessException(e.getMessage(), ErrorCode.FILE_UPLOAD_FAIL_ERROR); | |||
| } | |||
| } | |||
There was a problem hiding this comment.
코드 리뷰를 진행하겠습니다.
주요 변경 사항
uploadFile메소드의 매개변수에서uploaderId가 제거되었습니다. 이제 파일 업로드 시 업로더 ID를 받지 않게 되었습니다.- S3 키 생성 시
generateS3Key호출의 매개변수가 변경되어uploaderId가 아닌referenceId를 사용하게 되었습니다.
잠재적인 버그 위험
-
uploaderId제거:- 업로더 ID가 필요한 비즈니스 로직이 있었다면, 이 변경으로 인한 문제를 발생할 수 있습니다. 예를 들어, 파일 소유자를 추적할 수 없어질 수 있습니다.
-
referenceId:referenceId가null일 경우,generateS3Key메소드가 어떻게 동작하는지 확인이 필요합니다. 만약 이 값이 필수라면, 검증 로직이 추가되어야 합니다.
개선 제안
-
입력 검증 강화:
file,fileCategory,referenceId에 대한 검증을 강화하여 null 체크 및 유효성 검사를 포함하는 것이 좋습니다.
-
로그 추가:
- 메소드 시작 및 실패 시 로그를 추가함으로써 디버깅에 도움이 되도록 하는 것이 좋습니다. 특히 S3 업로드가 실패했을 경우 어떤 파일이 문제가 되었는지 로그로 남기는 것이 중요합니다.
-
예외 처리:
- 현재
Exception을 일반적으로 잡고 있습니다. 보다 구체적인 예외를 처리하여 더 정교한 에러 핸들링을 구현할 수 있습니다.
- 현재
-
문서화:
- 주석을 통해 메소드 변경 내용과 의도를 명확히 문서화하는 것이 좋습니다. 예를 들어,
uploaderId가 더 이상 필요하지 않은 이유 등을 추가하면 좋겠습니다.
- 주석을 통해 메소드 변경 내용과 의도를 명확히 문서화하는 것이 좋습니다. 예를 들어,
이러한 부분들을 고려하시어 코드를 수정하면 더욱 안정적이고 관리하기 쉬운 코드가 될 것입니다.
| .append(category) | ||
| .append("/") | ||
| .append(referenceId) | ||
| .append("/") | ||
| .append(fileName) | ||
| .toString(); | ||
| } |
There was a problem hiding this comment.
코드 패치에 대한 간단한 리뷰를 제공하겠습니다.
-
변수 이름 변경:
uploaderId가 메서드 시그니처에서 제거되었습니다. 이는 이 메서드가 이제 업로더 ID를 사용하지 않음을 나타냅니다. 만약 이 메서드가 여전히 업로더 ID와 관련된 기능을 원한다면, 해당 변수를 다시 포함시키는 것이 좋습니다.
-
메서드 시그니처 변경:
generateS3Key메서드의 원래 시그니처는(Long uploaderId, String category, String fileName)에서(String category, Long referenceId, String fileName)로 변경되었습니다. 이는 그에 따른 호출 부분에서도 모든 호출자가 수정되어야 함을 의미합니다. 호출하는 모든 코드가 올바르게 업데이트되었는지 확인하십시오. 그렇지 않으면 런타임 오류가 발생할 수 있습니다.
-
문서화 주석:
- 주석이 잘 작성되어 파일 경로에 대한 정보를 명확하게 설명하고 있습니다. 그러나
파일카테고리/참조ID/uuid파일명형식이 혼란스러울 수 있습니다. 예를 들어,uuid파일명은 UUID 형식에 대한 구체적인 설명을 보완하면 좋을 것입니다.
- 주석이 잘 작성되어 파일 경로에 대한 정보를 명확하게 설명하고 있습니다. 그러나
-
예외 처리:
- 입력 값에 대한 유효성 검사가 없는 것으로 보입니다.
category,referenceId,fileName의 값들이 null일 경우에 대한 예외 처리를 추가하는 것이 좋습니다. 그렇지 않으면 NPE(Null Pointer Exception)로 인해 프로그램이 비정상 종료될 수 있습니다.
- 입력 값에 대한 유효성 검사가 없는 것으로 보입니다.
-
성능:
StringBuilder를 사용할 이유는 문자열 결합 성능 향상입니다. 다만, 이와 관련된 성능 이점은 매우 미미할 수 있으며, 입력되는 문자열의 수와 크기에 따라 다를 수 있습니다. 필요한 경우 성능 테스트를 고려해볼 수 있습니다.
이와 같은 사항들을 염두에 두면서 코드를 보완하면 더욱 안정적이고 효율적인 코드가 될 것입니다. 추가적으로 다른 변수 또는 메서드와의 관계도 검토하여, 의도치 않은 부작용이 발생하지 않도록 주의해야 합니다.
| @@ -14,7 +14,7 @@ spring.datasource.username=${SPRING_DATASOURCE_USERNAME} | |||
| spring.datasource.password=${SPRING_DATASOURCE_PASSWORD} | |||
|
|
|||
| # JPA Settings | |||
| spring.jpa.hibernate.ddl-auto=create | |||
| spring.jpa.hibernate.ddl-auto=update | |||
| spring.jpa.generate-ddl=true | |||
| spring.jpa.show-sql=true | |||
| spring.jpa.properties.hibernate.format_sql=false | |||
There was a problem hiding this comment.
이 코드 패치는 Spring Boot 애플리케이션의 데이터베이스 설정 부분에서 JPA의 ddl-auto 설정을 변경하고 있습니다. 기존에는 create로 설정되어 있던 것이 update로 변경되었습니다. 이에 대한 코드 리뷰를 아래에 정리해 보았습니다.
코드 리뷰
-
설정 변경의 의도:
spring.jpa.hibernate.ddl-auto=create: 데이터베이스를 애플리케이션 시작 시마다 새로운 스키마로 생성합니다. 이미 데이터가 있는 경우 중요한 데이터가 삭제될 위험이 있습니다.spring.jpa.hibernate.ddl-auto=update: 데이터베이스 스키마의 업데이트를 수행하며, 기존 데이터를 유지합니다. 이는 운영 환경에서 더 안전한 설정입니다.
-
버그 리스크:
- 운영환경과 개발환경 차이: 개발 환경에서는
create를 사용하고, 프로덕션에서는update를 사용하는 것이 일반적입니다. 만약 이 설정이 잘못된 환경에 적용된다면 데이터 유실이나 비정상적인 동작이 발생할 수 있습니다. - 종속성과 호환성:
update는 모든 변경 사항을 자동으로 데이터베이스에 반영하지 않을 수 있습니다. 따라서, 일부 DB 스키마 변경이 자동으로 반영되지 않아서 수동으로 조정이 필요할 수 있습니다.
- 운영환경과 개발환경 차이: 개발 환경에서는
-
개선 제안:
- 환경 별 프로파일 사용: 개발, 테스트, 운영 환경 각각에 맞추어 적절한
ddl-auto설정을 환경 변수 또는 프로파일에 따라 다르게 지정할 수 있습니다. 예를 들어, 개발 환경에서는create, 운영 환경에서는none또는update를 사용하는 것이 좋습니다.# application-dev.properties spring.jpa.hibernate.ddl-auto=create # application-prod.properties spring.jpa.hibernate.ddl-auto=update
- 환경 별 프로파일 사용: 개발, 테스트, 운영 환경 각각에 맞추어 적절한
-
추가 설정 고려:
spring.jpa.show-sql=true는 쿼리 로그를 명시적으로 보여주는데, 이 설정은 개발 및 디버깅에 유용하지만, 운영 환경에서는 성능상 문제를 일으킬 수 있습니다. 로그 레벨 조정이 필요합니다.
이 변경은 데이터베이스의 유지를 고려할 때 좋은 판단이지만, 프로파일 및 환경 설정을 통한 접근이 더욱 안전한 방법이 될 것입니다.
| @@ -146,7 +142,7 @@ void testUploadFile_FileSizeExceed() { | |||
|
|
|||
| // When & Then | |||
| BusinessException exception = assertThrows(BusinessException.class, () -> { | |||
| fileService.uploadFile(mockFile, 1L, FileCategory.USER_PROFILE_IMAGE.name(), 1L); | |||
| fileService.uploadFile(mockFile, FileCategory.USER_PROFILE_IMAGE.name(), 1L); | |||
| }); | |||
|
|
|||
| assertEquals(ErrorCode.FILE_SIZE_EXCEED_ERROR, exception.getErrorCode()); | |||
There was a problem hiding this comment.
이 코드 패치에 대한 간단한 리뷰를 제공합니다.
개선 사항 및 버그 리스크
-
중복 파라미터 제거: 코드에서
uploadFile메소드 호출 시1L인자를 여러 번 제거했습니다. 이 부분은 불필요한 인자를 감소시켜 메소드를 더 간결하게 만드므로 긍정적인 변화입니다. 하지만 이러한 변경이 기존의 메소드의 시그니처를 변경하는 것인지 확인할 필요가 있습니다.uploadFile메소드의 구현이 무엇인지 보고, 이 변경으로 인한 부작용이 없는지 검증해야 합니다. -
유효성 검사:
INVALID_CATEGORY와 같은 직접적인 문자열을 코드에 남기는 것은 가독성과 유지보수성을 떨어뜨릴 수 있습니다. 이러한 상수들은FileCategory열거형에 포함되어 있거나 별도의 상수로 정의되어야 합니다. -
에러 코드의 하드코딩: 코드의 여러 부분에서 에러 코드가 하드코딩되어 있습니다. 이를 상수로 정의하면 코드 수정 시 발생할 수 있는 실수를 줄이고, 가독성을 향상시킬 수 있습니다.
-
테스트 코드의 견고함: 각 테스트가 의미하는 바를 명확히 하기 위해 더 구체적인 설명 주석을 추가하는 것이 좋습니다. 가독성을 높이고, 향후 다른 개발자가 코드를 이해하는 데 도움을 줄 수 있습니다.
-
파일 메타데이터 검증: 테스트 케이스에서 어떤 파일이 정상적으로 업로드되는지 확인하는 로직이 하나도 보이지 않습니다. 파일 업로드 성공 경로와 함께 실패 경로에 대한 테스트도 고려되어야 합니다. 성공적인 업로드 후에 반환된
APIResponse의 내용을 확인하는 테스트가 추가되면 좋겠습니다.
결론
이 코드 패치는 몇 가지 긍정적인 변화가 있지만, 메소드 호출 시의 인자 변경이 기존 로직에 미치는 영향을 주의 깊게 살펴봐야 합니다. 또한, 전반적인 코드 품질을 향상시키기 위한 몇 가지 제안이 있습니다.
| @@ -16,8 +14,6 @@ | |||
| import org.mockito.MockitoAnnotations; | |||
| import org.springframework.web.multipart.MultipartFile; | |||
|
|
|||
| import java.util.Optional; | |||
|
|
|||
| import static org.junit.jupiter.api.Assertions.*; | |||
| import static org.mockito.Mockito.*; | |||
|
|
|||
There was a problem hiding this comment.
코드 패치에 대한 간단한 리뷰를 제공하겠습니다.
-
파라미터 순서 변경:
generateS3Key메서드의 파라미터 순서가 변경되었습니다. 기존에는anyLong(), anyString()순서였으나, 변경된 코드에서는anyString(), anyLong()순서로 바뀌었습니다. 이 변경이 메서드의 동작에 맞는지 확인해야 합니다. 파라미터 순서가 잘못되면 의도한 대로 동작하지 않을 수 있으므로, 메서드가 정의된 부분도 함께 확인하는 것이 좋습니다.
-
테스트의 포괄성:
testUploadFile_Success와testUploadFile_S3UploadFailure두 테스트 케이스가 파일 업로드와 관련된 다양한 시나리오를 충분히 다루고 있는지 검토해야 합니다. 특별히 에러 상황에 대한 테스트 충분히 작성되어 있는지 확인할 필요가 있습니다.
-
예외 처리:
testUploadFile_S3UploadFailure에서RuntimeException을 발생시키는 코드가 추가되었습니다. 발생할 Exception에 대한 예상 및 처리 방안을 고려해야 합니다. 예를 들어, 특정한 예외 상황에서 사용자에게 어떻게 알릴지를 명확하게 해야 합니다.
-
메서드 호출 확인:
- 기능이 정확히 작동하는지 검사하기 위해 Mockito를 사용한 Verify를 통한 메서드 호출 확인을 추가하는 것이 좋습니다. 이를 통해 모든 메서드가 예상된 대로 호출되었는지 검사할 수 있습니다.
-
문서화:
- 각 테스트 메서드에 대한 설명 주석을 추가하는 것이 좋습니다. 어떤 시나리오를 테스트하는지, 어떤 기대 결과가 있는지를 문서화하면 나중에 코드 유지보수 시 유용합니다.
전반적으로, 코드 변경 사항은 명확하게 나타나 있지만, 변경의 맥락과 주변 코드의 관계를 잘 파악하여 추가 검증을 하는 것이 중요합니다. 항상 변경된 코드가 기존 기능과 일치하는지 확인하는 것이anner 중요합니다.
| public APIResponse uploadLogoImage(MultipartFile file, Long companyId, Long uploaderId) throws Exception{ | ||
| //1. 회사 id로 존재여부 판별 | ||
| Company companyEntity = companyRepository.findById(companyId) | ||
| .orElseThrow(() -> new BusinessException(ErrorCode.COMPANY_IS_NOT_FOUND)); |
There was a problem hiding this comment.
dev branch에 domain/admin/exception/CompanyNotFoundException.class 가 있어서 사용하시면 좋을 것 같습니다!
바로 MERGE하지 마세요! 검토 후 MERGE해야합니다!
✅ 최근 작업 주제 (하나 이상의 주제를 선택해주세요.)
🏆 구현 목표
회사 로고 업로드 기능 추가
📋 구현 사항 설명 (작업한 내용을 상세하게 기록합니다.)
🔍 테스트 방법 (변경 사항을 확인하기 위한 테스트 방법을 기술합니다.)
🛠️ 쓰이는 모듈
💬 기타 질문 및 특이 사항