Feature/company #13 #27
Conversation
- 회사 등록 Api 구현 - 예외처리(CompanyDuplicatedException.java) : 중복회사 조회 - 중복회사 에러코드(COMPANY_DUPLICATED_NUMBER) 추가 : 임의로 1000 부여, 추후 변경 예정
- 회사 등록 Api 구현 - 예외처리(CompanyDuplicatedException.java) : 중복회사 조회 - 중복회사 에러코드(COMPANY_DUPLICATED_NUMBER) 추가 : 임의로 1000 부여, 추후 변경 예정
- 등록 : 회사 추가하기 기능 - 예외처리 : 중복 시 예외처리 발생 확인
- Dto, Entity 는 EntityConverter.java 로 변환 - createCompany 대표 이미지 존재 여부는 파일 등록 시에 따라 변경되기 때문에 별도로 빌드
- 대표 이미지 등록 여부 외 정보 전달
…nto feature/company-#13 # Conflicts: # src/main/java/com/seveneleven/devlens/global/response/APIResponse.java
taekminKwon
requested review from
Eseas,
K-KY,
kdh10806,
HiSeungmin and
jeongns2611
as code owners
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| return APIResponse.success(SuccessCode.OK, paging.getContent()); | ||
| } | ||
|
|
||
| } |
There was a problem hiding this comment.
코드 패치 리뷰를 제공하겠습니다.
1. 전반적인 코드 구조
- 클래스와 메서드의 구조가 명확하며, 의미 있는 이름을 사용하고 있습니다. RESTful API의 일반적인 규칙을 준수하고 있어 이해하기 쉽습니다.
2. 버그 리스크
-
null 검증:
createCompany및readCompany메서드에서companyDto와companyId에 대한 null 검증이 필요할 수 있습니다. valid annotation을 사용하고 있지만, 비즈니스 로직에서 null이 전달될 가능성에 대비하는 것이 좋습니다. -
예외 처리:
companyReadService나companyCreateService에서 발생할 수 있는 예외를 처리하는 로직이 없습니다. 예를 들어, 회사 ID가 존재하지 않을 경우 적절한 예외를 던지고 사용자에게 알릴 수 있는 방식이 필요합니다.
3. 개선 제안
-
API 응답 통일성:
readCompany메서드에서 응답 메시지는 하드코딩되어 있습니다. API 응답 메시지를 상수로 관리하거나, 국제화(i18n)를 고려해 다국어 지원을 할 수 있습니다. -
형식화된 반환 값:
readAllCompanies메서드에서 페이지네이션 정보를 포함한 응답을 반환하면 클라이언트 측에서 더 많은 정보를 제공할 수 있습니다. 예를 들어, 총 페이지 수, 현재 페이지, 전체 데이터 수 등을 함께 포함할 수 있습니다. -
RESTful URL 디자인:
readAllCompanies메서드의 URL을/companies로 변경하면 더 RESTful하게 보일 수 있습니다. 주의 깊게 고려해 보시기 바랍니다.
4. 사용할 수 있는 추가 라이브러리
- 검증 실패 메세지:
@Valid를 사용할 때 검증 실패 시 발생하는 예외에 대한 커스터마이징을 고려할 수 있습니다. 예를 들어, 어떤 필드에서 문제가 발생했는지 명확히 알리는 방법입니다.
결론
전반적으로 잘 작성된 코드이며, 일부 예외 처리 및 유지 보수성을 높일 수 있는 작은 개선 사항들이 있습니다. 주요 비즈니스 로직에 대해 테스트를 잘 작성하여 안정성을 확보하는 것도 중요합니다.
| .activeStatus(companyDto.getActiveStatus()) | ||
| .build(); | ||
| } | ||
| } |
There was a problem hiding this comment.
코드 리뷰에 대한 의견을 드리겠습니다.
-
Null 체크:
toDTO및toEntity메서드에서 입력 파라미터에 대해 null 체크가 없습니다. 만약 null 객체가 들어온다면 NPE(Null Pointer Exception)가 발생할 수 있습니다. 입력 값에 대한 유효성 검사를 추가하는 것이 좋습니다. -
객체 변환 로직의 중복: 두 메서드에서 많은 코드가 중복되고 있습니다.
Company객체와CompanyDto객체 간의 변환에 대한 공통 로직을 분리하여 재사용할 수 있습니다. 예를 들어, 변환을 위한 정적 메서드를 별도의 유틸리티 클래스로 두거나, 변환 로직을 별도의 메서드로 따로 빼는 방법을 고려할 수 있습니다. -
Optional 활용: 만약
CompanyDto의 필드 중 일부가 선택적으로 존재할 수 있다면, Optional을 활용하여 다루는 것이 코드의 가독성과 안전성을 높일 수 있습니다. -
정상 및 오류 상황에 대한 처리: 객체의 필드 값이 기대한 대로 들어오지 않을 경우를 대비해, 에러 핸들링 로직을 추가하는 것이 중요합니다. 예를 들어, 비즈니스 등록 번호가 잘못된 형식일 경우 예외처리를 통해 사용자에게 오류 메시지를 전달해야 할 필요가 있습니다.
-
JUnit 테스트: 변환 로직에 대한 유닛 테스트가 포함되어 있지 않은 것 같습니다. 적절한 테스트 케이스를 작성하여 변환 로직이 올바르게 작동하는지 검증해야 합니다.
-
문서화: 코드에 대한 설명이 부족합니다. 클래스와 메서드에 JavaDoc을 추가하여 목적과 기능을 문서화하는 것이 좋습니다.
이러한 점들을 개선하면 코드의 안정성과 가독성을 높일 수 있을 것입니다.
| public interface CompanyRepository extends JpaRepository<Company, Long> { | ||
| Company findByBusinessRegistrationNumber(String businessRegistrationNumber); | ||
| Page<Company> findAll(Pageable pageable); | ||
| } No newline at end of file |
There was a problem hiding this comment.
코드 리뷰 결과는 다음과 같습니다:
-
패키지 조직:
package com.seveneleven.devlens.domain.admin.db;에서 패키지 구조가 잘 구성되어 있지만, 만약admin과 관련된 리포지토리라면admin패키지 아래에 있는 것이 합리적입니다. 만약 여러 도메인이 보유한 일반적인 리포지토리라면 더 적합한 패키지 구조를 고려하는 것이 좋습니다. -
Null 검증:
@NonNull어노테이션이 사용되지 않았는데, 메서드 인자로 넘어오는 값(null값 포함)의 검증을 추가하는 것이 좋습니다. 예를 들어,findByBusinessRegistrationNumber메서드에 대한 입력 검증을 고려할 수 있습니다. -
페이징 처리:
findAll(Pageable pageable)메서드는 JpaRepository에 이미 정의된 메서드입니다. 따라서, 명시적으로 오버라이드할 필요가 없으며, 이를 삭제하여 간결하게 작성할 수 있습니다. -
주석 및 문서화: 각 메서드에 대한 설명이 없는데, 이는 나중에 코드의 가독성을 떨어뜨릴 수 있습니다. 특히 리포지토리 인터페이스에서는 각 메서드의 목적과 사용 방법에 대한 주석을 추가하는 것이 좋습니다.
-
새로운 줄: 파일 끝에 새로운 줄이 부족합니다. 이는 일부 버전 관리 시스템에서 경고로 표시될 수 있습니다. 마지막에 빈 줄을 추가해주세요.
-
예외 처리:
findByBusinessRegistrationNumber메서드는 해당 비즈니스 등록 번호가 존재하지 않는 경우 null을 반환할 수 있습니다. 이로 인해 호출하는 측에서 NullPointerException이 발생할 수 있으니, 필요에 따라 예외를 던지도록 변경하는 것도 좋습니다.
이러한 사항들을 고려하여 코드의 품질을 높이고 안전성을 강화하는 것이 좋습니다.
| public interface EntityConverter <DTO,ENTITY>{ | ||
| DTO toDTO(ENTITY entity); | ||
| ENTITY toEntity(DTO dto); | ||
| } |
There was a problem hiding this comment.
이 코드 패치는 EntityConverter 인터페이스를 정의하고 있습니다. 간략한 코드 리뷰를 해보겠습니다.
장점
-
제네릭 사용: DTO와 ENTITY 타입을 제네릭으로 정의하여 다양한 데이터 전송 객체와 엔티티를 변환할 수 있도록 하고 있습니다. 이는 코드의 재사용성을 높이고 타입 안전성을 제공합니다.
-
명확한 메서드 정의:
toDTO와toEntity메서드가 이 인터페이스에서 어떤 기능을 수행하는지 명확하게 나타내고 있습니다.
개선 제안
-
주석 추가: 인터페이스 및 각 메서드에 대한 설명을 주석으로 추가하면 이 코드를 사용하는 개발자가 이해하는 데 도움이 될 것입니다. 예를 들어, 어떤 때에 이 메서드를 사용해야 하는지에 대한 설명을 추가할 수 있습니다.
-
예외 처리: 변환 과정에서 발생할 수 있는 예외사항에 대한 처리를 고려할 수 있습니다. 메서드 시그니처에 예외를 던지는 경우를 명시하거나, 변환이 실패했을 때의 처리를 정의할 수 있습니다.
-
디자인 패턴: 이 인터페이스를 구현하는 클래스에서 Singleton 디자인 패턴을 사용할 경우, 불필요한 인스턴스 생성을 막을 수 있습니다. 이 점을 문서화하면 좋을 것입니다.
-
실제로 사용되는 컨텍스트: 이 인터페이스가 어떤 패턴이나 아키텍처에서 사용할 것인지에 대한 정보를 추가하면 좋습니다. 예를 들어, MVC 구조에서 데이터 변환이 필요한 경우와 같은 특정 상황을 예로 들 수 있습니다.
버그 위험
현재 코드에서는 버그의 위험성은 낮지만, 만약 DTO와 ENTITY가 서로 간에 변환하는 과정에서 필드의 불일치나 데이터 손실이 발생할 수 있습니다. 이 부분에 대한 검증 로직이 추후에 필요할 수 있습니다.
이와 같은 개선 사항을 고려하면 코드의 품질과 유지 보수성이 향상될 것입니다.
| public CompanyDuplicatedException() { | ||
| this(COMPANY_DUPLICATED_NUMBER.getMessage()); | ||
| } | ||
| } |
There was a problem hiding this comment.
이 코드 패치에 대한 간단한 코드 리뷰를 제공하겠습니다.
-
예외 처리:
CompanyDuplicatedException은BusinessException을 확장하여 특정 도메인에서 발생할 수 있는 예외를 정의하고 있습니다. 이는 비즈니스 로직에서 예외를 처리하는 데 유용합니다.
-
상수 사용:
COMPANY_DUPLICATED_NUMBER를 통해 에러 코드를 관리하고 있는데, 이는 코드의 가독성을 높여줍니다. 그러나 이 상수가 어떤 값인지, 그리고 어떻게 설정되는지에 대한 정보가 없어 약간의 혼동이 있을 수 있습니다. 주석을 추가해주면 더 좋을 것 같습니다.
-
생성자:
- 두 개의 생성자가 제공되고 있는데, 기본 생성자는 상수에서 메시지를 가져오도록 잘 설정되어 있습니다. 하지만 이러한 기본 메시지가 항상 적절할지에 대한 명확한 검토가 필요합니다. 필요한 경우 특정한 메시지를 사용자 정의할 수 있도록 하는 것도 고려해보세요.
-
코드 스타일:
- 코드 스타일은 일반적으로 일관되지만, Java 클래스의 경우 패키지 선언 뒤에 항상 빈 줄을 추가하는 것이 관례적입니다.
-
테스트 케이스:
- 이 예외를 사용하는 부분에서 적절한 테스트 케이스를 작성해야 합니다. 예외가 발생하는 상황과 처리하는 로직을 충분히 테스트해야 시스템의 안정성을 높일 수 있습니다.
개선 제안:
- 다국어 지원이 필요할 경우, 에러 메시지를 하드코딩하지 않고 리소스에서 읽어오는 방식으로 변경하는 것을 고려하면 좋습니다.
- 각 생성자의 목적과 사용법에 대한 설명 주석을 추가해주면 코드의 가독성이 더 올라갈 것입니다.
- 예외 클래스에 사용자 정의 속성을 추가하는 것도 고려해볼 수 있습니다.
이와 같은 사항들을 고려하여 코드를 보강하면 더욱 안정적이고 유용한 예외 처리가 될 것입니다.
| .activeStatus(this.activeStatus) | ||
| .build(); | ||
| } | ||
| } |
There was a problem hiding this comment.
코드 리뷰를 진행하겠습니다.
장점
- Lombok 사용:
Getter,Setter,ToString,Builder,NoArgsConstructor,AllArgsConstructor등의 어노테이션을 사용하여 코드가 간결해졌습니다. - Builder 패턴:
returnCompanyStatus메서드에서 Builder를 사용하여 객체를 생성하는 방식은 가독성이 좋고 유연한 객체 생성을 가능하게 합니다.
개선 사항 및 버그 리스크
-
메서드 명세:
returnCompanyStatus()는 현재 객체의 상태를 반환하는 역할을 하지만, 메서드 이름이returnCompanyStatus로 되어 있어 혼란을 줄 수 있습니다.createCopy()또는clone()과 같은 이름이 좀 더 명확할 수 있습니다.
-
Boolean 타입 사용:
Boolean타입 대신boolean원시 타입을 사용하는 것이 좋습니다. 불리언 값이 null일 수 없으므로, 필요시 체크를 통해 기본값을 사용할 수 있습니다.representativeImageExists와activeStatus는 아마도 논리적인 진위를 나타내기 때문에,boolean으로 선언하는 것이 더 적합할 것입니다.
-
null 체크:
returnCompanyStatus()와 같은 메서드에서 필드 값이 null일 가능성에 대한 처리가 필요합니다. 필드 값의 null 여부를 체크하고 기본값을 설정할 수 있도록 고려해야 합니다.
-
단위 테스트:
- 이 DTO를 사용하는 코드에 대해 단위 테스트를 작성하여 올바른 값이 설정되고 생성되는지 확인하는 것이 좋습니다.
-
문서화:
- 클래스와 메서드에 대한 주석을 추가하여 코드의 용도 및 동작을 설명하면 다른 개발자가 이해하는 데 도움이 됩니다.
결론
전반적으로 코드가 잘 작성되었으나, 메서드의 명명, 데이터 타입 선택, null 체크 등을 개선할 여지가 있습니다. 제품의 안정성과 유지보수성을 높이기 위해 이러한 사항들을 고려하면 좋겠습니다.
| throw new CompanyDuplicatedException(); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
이 코드 패치에 대한 간단한 리뷰는 다음과 같습니다.
버그 위험
- 예외 처리:
createCompany메서드에서CompanyDuplicatedException외의 다른 예외가 발생할 경우에 대한 처리가 없습니다. 만약 다른 예외가 발생하면, 이 메서드를 호출한 쪽에서 예외의 원인을 파악하기 어려울 수 있습니다. 모든 예외를 한 곳에서 처리하고 로깅을 추가하는 것이 좋습니다.
개선 제안
-
메서드 주석: 메서드 주석에 적힌 함수 목적 문구가 자세히 설명되고 있지만, 일반적으로 함수의 반환값이나 파라미터에 대한 설명도 포함하면 좋습니다. 현재 주석은 너무 간단합니다.
-
비즈니스 로직 분리: 비즈니스 로직을 분리하는 것이 좋습니다. 예를 들어, 회사 객체를 생성하는 코드를 별도의 메서드로 분리하면 가독성이 높아지고, 테스트가 용이해집니다.
-
레플리케이션:
foundCompany가null인지 판별하는 부분에서 자바 8 이상의 Optional을 활용할 수 있습니다. 이를 통해 더 깔끔한 코드를 작성할 수 있습니다.Optional<Company> foundCompany = Optional.ofNullable(companyRepository.findByBusinessRegistrationNumber(businessRegistrationNumber)); if (foundCompany.isPresent()) { throw new CompanyDuplicatedException(); }
-
컴파일러 경고:
var키워드를 사용할 때, 가독성이 떨어질 수 있습니다. 특히 타입이 명확하지 않은 경우에는 명시적인 타입을 사용하는 것이 좋습니다. -
메서드 네이밍:
createCompany메서드는 직관적이지만, 메서드가 수행하는 정확한 내용을 좀 더 설명적으로 나타낼 수 있습니다. 예를 들어registerNewCompany와 같이 명명할 수 있습니다. -
Validation:
companyDto의 필드 검증 (예: 이메일 형식, 필수 필드 체크 등) 이 필요할 수 있습니다. 이를 위해 별도의 검증 로직을 추가하는 것이 좋습니다.
이러한 개선 사항들을 고려하면 코드의 품질과 유지 보수성을 높일 수 있습니다.
| Page<Company> companyPage = companyRepository.findAll(pageable); | ||
| return companyPage.map(companyConverter::toDTO); | ||
| } | ||
| } |
There was a problem hiding this comment.
코드 리뷰에 대해 말씀드리겠습니다.
-
에러 메시지의 구체성:
EntityNotFoundException을 던질 때 "회사 정보를 찾을 수 없습니다."라는 메시지를 사용하고 있습니다. 여기서id값도 함께 로그로 남기면 디버깅 시 유용할 수 있습니다. 예를 들어:.orElseThrow(() -> new EntityNotFoundException("회사 정보를 찾을 수 없습니다. id: " + id));
-
매직 넘버:
getListOfCompanies메서드에서 페이지 크기10을 매직 넘버로 직접 사용하고 있습니다. 이를 상수로 정의하여 코드의 가독성을 높이고, 나중에 변경이 필요할 때 수정을 쉽게 할 수 있도록 하는 게 좋습니다.private static final int PAGE_SIZE = 10;
-
페이지 번호 검증:
getListOfCompanies메서드의page파라미터에 대해 검증이 없습니다. 음수일 경우 예외를 던지도록 예외 처리를 추가하면 좋습니다.
-
트랜잭션 범위:
@Transactional(readOnly = true)를 사용하여 읽기 전용을 명시한 것은 좋습니다. 그러나 이 메서드가 다른 메서드에서 호출될 때, 비즈니스 로직에 따라 적절한 관리가 필요할 수 있습니다. 다중 트랜잭션이 생길 수 있는 경우 이를 고려해야 합니다.
-
JavaDoc:
- 각 메서드에 대한 JavaDoc 주석을 추가하여 메서드의 목적, 파라미터, 반환 값 등을 명세하면 사용자나 다른 개발자가 이해하는 데 도움이 됩니다.
-
DTO 변환의 일관성:
- DTO 변환이 여러 곳에서 필요할 수 있으므로,
CompanyConverter의 메서드에 대해 논리적인 그룹을 만드는 것도 좋습니다. 예를 들어,toDTO()외에 다른 변환 메서드가 필요할 경우를 대비해 유지보수성을 높일 수 있습니다.
- DTO 변환이 여러 곳에서 필요할 수 있으므로,
이상으로 코드 리뷰를 마치겠습니다. 전반적으로 코드가 깔끔하고 잘 구조화되어 있습니다. 위의 몇 가지 사항을 고려하면 더욱 품질 높은 서비스가 될 것입니다.
| @@ -10,6 +10,7 @@ public enum ErrorCode { | |||
|
|
|||
| // 1000번대 코드 : 회원 관련 | |||
| UNAUTHORIZED(1000, HttpStatus.UNAUTHORIZED, "사용자 인증이 필요합니다."), | |||
| COMPANY_DUPLICATED_NUMBER(1000, HttpStatus.BAD_REQUEST,"이미 등록된 회사입니다."), | |||
|
|
|||
| // 2000번대 코드 : 프로젝트 관련 | |||
|
|
|||
There was a problem hiding this comment.
코드 패치에 대한 간단한 리뷰를 진행하겠습니다.
-
목적 확인: 추가된
COMPANY_DUPLICATED_NUMBER오류 코드는 이미 등록된 회사에 대한 에러를 나타내고 있습니다. 에러 코드가 1000번대에 포함되므로 회원 관련 에러와 섞일 수 있을 주의가 필요합니다. -
HTTP 상태 코드:
HttpStatus.BAD_REQUEST는 적절한 선택입니다만, 추가된 오류 코드가 회원 관련인지 아니면 회사 등록과 관련된 것인지 명확히 해야 합니다. 예를 들어, 회사 관련 오류라면 2000번대에 넣는 것이 더 나을 수 있습니다. -
문서화: 코드에 주석을 추가하여
COMPANY_DUPLICATED_NUMBER가 어떤 상황에서 발생하는지 설명하는 것이 좋습니다. 이는 코드 유지보수와 다른 개발자의 이해를 돕습니다. -
확장성: 향후에 더 많은 오류 코드를 추가할 가능성이 있는 경우, 관련 오류를 그룹화하여 관리하기 용이하도록 1000번대와 2000번대 오류 코드를 명확하게 구분하는 것이 좋습니다.
-
테스트: 새로운 오류 코드를 추가했으므로, 해당 코드가 사용되는 곳에서 적절히 테스트하고 예상한 대로 동작하는지 확인해야 합니다.
이 외에도 코드의 일관성을 유지하고, 의미를 명확히 하도록 주의하는 것이 좋습니다. 추가적인 질문이 있다면 언제든지 말씀해 주세요!
| verify(companyRepository, times(1)).findByBusinessRegistrationNumber(duplicatedBusinessNumber); | ||
| verify(companyRepository, never()).save(any(Company.class)); // save는 호출되지 않아야 함 | ||
| } | ||
| } No newline at end of file |
There was a problem hiding this comment.
코드 리뷰를 진행하겠습니다.
-
테스트 클래스 구조:
@Mock애노테이션을 사용하여companyService를 모킹하는 것은 적절하지 않아 보입니다. 테스트의 목적은CompanyCreateService의 메소드를 검증하는 것이므로, 실제 서비스 인스턴스를 사용하는 것이 더 좋습니다.CompanyService의 인스턴스를 실체화하거나, 필요한 경우@InjectMocks애노테이션을 사용해 주입해야 합니다.
-
전역 변수 사용:
- 각 테스트 메소드 안에서 사용하는 변수를 전역 변수로 설정하고, 메소드 내에서 재초기화하는 방식이 좋습니다. 이렇게 함으로써, 메소드의 독립성을 유지할 수 있습니다.
-
버그 위험:
createCompany메소드에서CompanyDto를 통해 생성되는Company객체의 상태가companyRepository.save()호출 이전에 설정되어야 합니다. 현재 상태와 관련이 없는, 모든 필드를 강제로 설정하는 로직이 들어가 있습니다. 이는 나중에 다른 테스트에서 사이드 이펙트를 유발할 수 있습니다.
-
예외 처리 테스트:
- 중복 검사 메소드에서
companyConverter.toEntity(companyDto)호출을 Mock 처리하지 않은 것처럼 보입니다. 해당 변환 로직이 어떤 방식으로 동작하는지에 따라 예외가 발생할 수 있습니다. 이 부분도 Mock을 추가하여 테스트의 신뢰성을 높이는 것이 좋습니다.
- 중복 검사 메소드에서
-
assertEquals 사용:
- 필드별로
assertEquals를 여러 번 호출하는 대신, 전체 객체에 대해 비교하는 메소드를 작성하거나,equals메소드를 오버라이드하여 한번에 비교할 수 있는 방법을 고려해보세요. 코드의 가독성을 높일 수 있습니다.
- 필드별로
-
주석 사용:
- 각 섹션에 대한 설명이 명확히 되어 있는 것은 좋습니다. 하지만 메소드에 대한 설명 주석을 추가하여, 그 메소드의 목적과 기대 결과를 명확히 하는 것도 좋습니다.
-
코드 스타일:
var를 사용한 점은 Java 10 이상에서는 유용하지만, 명시적인 타입이 보다 읽기 쉬운 코드를 제공할 수 있습니다. 팀의 코딩 스타일 가이드에 따라 이 부분을 조정하세요.
전반적으로 테스트의 목적이 명확하고, 기능적 요구 사항을 충족하는지 확인하려는 노력이 보입니다. 그러나 위의 몇 가지 세부적으로 다루어야 할 사항들을 반영하면 더욱 견고한 테스트 코드가 될 것입니다.
|
|
||
| import java.util.List; | ||
|
|
||
| @Slf4j |
| .companyName(companyDto.getCompanyName()) | ||
| .representativeName(companyDto.getRepresentativeName()) | ||
| .representativeEmail(companyDto.getRepresentativeEmail()) | ||
| .representativeContact(companyDto.getRepresentativeContact()) | ||
| .address(companyDto.getAddress()) | ||
| .businessType(companyDto.getBusinessType()) | ||
| .businessRegistrationNumber(companyDto.getBusinessRegistrationNumber()) | ||
| .representativeImageExists(companyDto.getRepresentativeImageExists()) | ||
| .activeStatus(companyDto.getActiveStatus()) | ||
| .build(); |
There was a problem hiding this comment.
엔티티를 이렇게 변환 하면 새로운 엔티티가 생성 됩니다. 의도 하신 것 일 수 도 있지만 그렇지 않다면 id가 같이 들어가야 합니다.
| public CompanyDto returnCompanyStatus() { | ||
| return CompanyDto.builder() | ||
| .companyName(this.companyName) | ||
| .representativeName(this.representativeName) | ||
| .representativeContact(this.representativeContact) | ||
| .representativeEmail(this.representativeEmail) | ||
| .address(this.address) | ||
| .businessType(this.businessType) | ||
| .businessRegistrationNumber(this.businessRegistrationNumber) | ||
| .representativeImageExists(this.representativeImageExists) | ||
| .activeStatus(this.activeStatus) | ||
| .build(); | ||
| } |
There was a problem hiding this comment.
이 부분이 어떤 의도인지 잘 모르겠습니다.
returnCompanyStatus() 를 호출 할 수 있다면 이미 CompanyDto를 알고 있는 상태인데 그냥 그 객체를 리턴하면 되지 않을까요?
| @GetMapping("/{companyId}") | ||
| public APIResponse<CompanyDto> readCompany(@PathVariable Long companyId) { | ||
| var company = companyReadService.getCompanyDto(companyId); | ||
| return APIResponse.success(SuccessCode.OK, "회사 조회가 완료되었습니다.",company); | ||
| } |
There was a problem hiding this comment.
현재 companyDto는 companyId를 리턴하고 있지 않습니다.
프론트는 companyId를 모르기 때문에 이 API를 호출 할 수 없습니다.
|
|
||
| @GetMapping("") | ||
| public APIResponse<List<CompanyDto>> readAllCompanies(@RequestParam(value = "page", defaultValue = "0") int page) { | ||
| Page<CompanyDto> paging = this.companyReadService.getListOfCompanies(page); |
There was a problem hiding this comment.
추가로 페이징 처리한 것을 List로 반환하는 것 역시 맞지 않습니다. 페이징 객체에 프론트에서 필요한 정보들이 들어있습니다.
| @GetMapping("") | ||
| public APIResponse<List<CompanyDto>> readAllCompanies(@RequestParam(value = "page", defaultValue = "0") int page) { | ||
| Page<CompanyDto> paging = this.companyReadService.getListOfCompanies(page); | ||
| return APIResponse.success(SuccessCode.OK, paging.getContent()); |
There was a problem hiding this comment.
SuccessCode가 OK라면 데이터만 입력해서 반환해도 괜찮습니다.
| } catch (CompanyDuplicatedException e) { | ||
| throw e; | ||
| } |
| Company foundCompany = companyRepository.findByBusinessRegistrationNumber(businessRegistrationNumber); | ||
| if (foundCompany != null) { | ||
| throw new CompanyDuplicatedException(); |
There was a problem hiding this comment.
Repository에서 Optional을 사용한 후에 orElseThrow를 사용하는 게 더 좋아보입니다.
- 회원 관련 Entity 어노테이션 수정 - 상태, YN Enum 타입 추가 - id 컬럼명, 변수명 수정
- 회원 ID, Email 컬럼 수정
- 사업자 유형 개인/법인 타입으로 수정 - enum에 @Getter 추가
| PaginatedResponse<CompanyDto.CompanyResponse> response = new PaginatedResponse<>( | ||
| paging.getContent(), | ||
| paging.getNumber(), | ||
| paging.getSize(), | ||
| paging.getTotalElements(), | ||
| paging.getTotalPages(), | ||
| paging.isLast() | ||
| ); |
There was a problem hiding this comment.
PaginatedResponse 안에 메서드를 생성자가 아닌, 팩토리 메서드로 만드는 게 좋아보입니다.
Page 객체만 매개변수로 넣어도 될 것 같습니다
| @GetMapping("") | ||
| public APIResponse<PaginatedResponse<CompanyDto.CompanyResponse>> readAllCompanies(@RequestParam(value = "page", defaultValue = "0") int page) { | ||
| Page<CompanyDto.CompanyResponse> paging = companyReadService.getListOfCompanies(page); | ||
| PaginatedResponse<CompanyDto.CompanyResponse> response = new PaginatedResponse<>( | ||
| paging.getContent(), | ||
| paging.getNumber(), | ||
| paging.getSize(), | ||
| paging.getTotalElements(), | ||
| paging.getTotalPages(), | ||
| paging.isLast() | ||
| ); |
There was a problem hiding this comment.
클라이언트 측에서는 몇페이지까지 있는지 모르기 때문에 무작정 호출 하는 수 밖에 없어요.
총 패이지 수를 리턴하는 API가 있으면 좋을 것 같습니다.
| private final CompanyReadService companyReadService; | ||
| private final CompanyResponseConverter companyResponseConverter; | ||
|
|
||
| @PostMapping("/create") |
There was a problem hiding this comment.
REST API 명명 규칙에 따라서 동사는 되도록 지양하는 편이 좋습니다.
'/create'를 지우는 게 좋아보입니다
아무것도 안넣는게 더 혼란을 줄 수 있는 것 같습니다.
관념상 domain/company 가 회사를 생성하는 API라고 생각하는 사람은 적을 것 같아요
동사를 지양해야 한다고 한다면 new나 newcompany 를 넣는게 좋을 것 같습니다.
- handleCompanyDuplicatedException 메서드 추가
…nto feature/company-#13 # Conflicts: # src/main/java/com/seveneleven/devlens/domain/member/entity/Company.java
| import lombok.NoArgsConstructor; | ||
| import org.springframework.data.domain.Page; | ||
|
|
||
| import java.util.List; |
There was a problem hiding this comment.
코드 리뷰 결과는 다음과 같습니다:
-
제너릭 타입 간소화: 제너릭 타입을 간소화하기 위해
PaginatedResponse<CompanyDto.CompanyResponse>대신PaginatedResponse<>사용한 것은 좋은 수정입니다. 이는 코드 가독성을 높이며 중복을 줄입니다. -
불필요한 띄어쓰기 제거: 기존의 코드에서 공백이 한 줄 있었는데, 이는 제거되었습니다. 이는 코드의 일관성을 유지하는데 도움을 줍니다.
-
매개변수 검사:
createPaginatedResponse메소드에 전달되는page객체가 null일 경우에 대한 검사가 없습니다. 이를 방지하기 위해 null 검사를 추가하는 것이 좋습니다. 예를 들어:if (page == null) { throw new IllegalArgumentException("Page cannot be null"); }
-
주석 추가: 메소드의 기능이나 용도에 대한 주석이 없으므로, 이 메소드가 무엇을 하는지 명확하게 이해할 수 있도록 간단한 주석을 추가하는 것이 좋습니다.
-
예외 처리: 내부 로직에서 발생할 수 있는 예외 상황에 대한 처리가 없으므로, 필요시 예외를 처리하거나 상위로 전파하는 로직을 고려해야 합니다.
-
타입 안정성:
Page<CompanyDto.CompanyResponse>의 타입이 정확하게 맞는 지 확인하고, 다른 타입의 페이지가 넘어오는 것을 방지할 수 있는 타입 안정성 점검을 강화하는 것이 좋습니다.
최종적으로, 위의 제안들을 적용하면 코드의 안정성과 가독성을 높일 수 있습니다.
| import com.seveneleven.devlens.domain.admin.dto.CompanyDto; | ||
| import com.seveneleven.devlens.domain.member.constant.YN; | ||
| import com.seveneleven.devlens.domain.member.entity.Company; | ||
| import com.seveneleven.devlens.global.response.ErrorCode; |
There was a problem hiding this comment.
코드 패치에 대한 간단한 코드 리뷰를 진행하겠습니다.
-
변수 사용:
pageSize변수가 정의되어 있지 않은 경우, 코드가 컴파일 에러를 일으킬 수 있습니다.pageSize의 값을 어떤 곳에서 설정했는지 확인해야 합니다. 해당 변수의 범위(scope)가 올바르게 설정되어 있는지 살펴보세요. -
예외 처리:
getListOfCompanies메서드에서 페이지 번호가 음수일 경우 예외 처리가 필요할 수 있습니다. 이러한 경우PageRequest.of(page, pageSize, ...)호출에서IllegalArgumentException이 발생할 수 있으므로, 이를 사전에 체크하고 적절한 에러 메시지를 반환하는 것이 좋습니다. -
주석: 메서드에 대한 설명 주석이 부족해 보입니다. 이 메서드는 특정 페이지의 회사 목록을 가져오는 기능이므로, 그에 대한 간단한 설명을 추가하는 것이 코드 가독성을 높이는 데 도움이 될 것입니다.
-
정렬 기준: 정렬 기준으로
"companyName"이 하드코딩되어 있습니다. 때에 따라 요구 사항이 달라질 수 있으므로, 정렬 필드를 파라미터로 받아 유연성을 더하는 것이 좋습니다. -
페이지 크기 조정:
pageSize라는 변수를 사용하고 있으나, 이는 하드코딩된 10이라는 숫자를 바꾼 것일 뿐입니다. 만약 페이지 크기 변경이 자주 필요하다면, 설정 파일에서 읽어오는 방법으로 리팩토링할 수 있습니다. 이를 통해 유지보수성을 높일 수 있습니다.
이러한 점들을 고려하여 코드를 수정하시면, 더 안전하고 가독성이 좋은 코드를 만들 수 있을 것입니다.
…nto feature/company-#13
| import org.springframework.data.domain.Page; | ||
| import org.springframework.validation.annotation.Validated; | ||
| import org.springframework.web.bind.annotation.*; | ||
|
|
There was a problem hiding this comment.
코드 패치를 리뷰하겠습니다. 여러 가지 점에서 개선할 부분과 잠재적인 버그 위험 요소를 제시하겠습니다.
-
주석 제거:
readCompanyTotalPages메서드에 대한 설명 주석이 제거되었습니다. 메서드의 목적과 사용 방법에 대한 설명 주석을 남겨두는 것이 좋습니다. 다른 개발자가 코드를 이해하는 데 도움이 됩니다. -
기능 축소:
readCompanyTotalPages메서드가 삭제되었습니다. 이 메서드는 회사 전체 페이지 수를 조회하는 기능을 제공하는데, 만약 이 기능이 다른 부분에서 필요하다면 나중에 다시 구현해야 할 수 있습니다. 이를 고려하여 이 기능이 필요한 경우 주석이나 TODO를 추가해 두는 것이 좋습니다. -
API의 일관성: API의 endpoints 의 일관성을 검토해야 합니다. 전체 페이지 수를 조회하는 API가 필요하다면, 제거하는 것보다 이름을 바꾸거나 refactor 하는 것이 더 좋습니다.
-
에러 처리: 데이터 조회 시 예외가 발생할 가능성이 있으니, 이러한 경우에 대한 에러 처리 로직이 필요할 수 있습니다. 현재 코드는 그러한 처리가 없는 것으로 보입니다.
-
테스트 케이스: 삭제된 메서드에 대한 테스트 케이스가 존재하는 경우, 코드 패치 후 메서드가 제거되면서 관련된 테스트도 함께 삭제되었는지 확인해야 합니다. 기존 기능의 동작을 유지하기 위한 단위 테스트가 필요할 수 있습니다.
결론적으로, 코드의 가독성과 유지보수성을 향상시키기 위해 주석을 활용하고, API의 일관성을 유지하며, 가능한 모든 에러에 대한 처리를 고려하는 것이 좋습니다.
| import com.seveneleven.devlens.domain.member.entity.Company; | ||
| import com.seveneleven.devlens.global.response.ErrorCode; | ||
| import jakarta.persistence.EntityNotFoundException; | ||
| import lombok.RequiredArgsConstructor; |
There was a problem hiding this comment.
코드 리뷰를 진행하겠습니다. 다음은 코드 패치에 대한 간단한 평가와 개선 사항입니다.
-
기능 제거:
getTotalPages()메서드가 주석 처리되어 삭제된 것으로 보입니다. 이 메서드는 전체 페이지 수를 계산하는 기능을 하므로, 페이지네이션을 사용하는 경우 중요한 기능입니다. 만약 이 기능이 더 이상 필요 없거나 다른 방법으로 처리될 계획이라면, 그에 대한 명확한 문서화가 필요합니다. -
Transaction 설정:
@Transactional(readOnly = true)어노테이션이 제거되었습니다. 이 어노테이션은 데이터베이스에서 읽기 전용 트랜잭션을 보장하는데, 이는 성능 최적화에 도움이 됩니다. 메서드가 데이터 수정 작업을 포함하지 않는 경우에는 이 설정을 유지하는 것이 좋습니다. -
페이지 크기 변수:
pageSize변수가 어디에서도 정의되지 않았습니다. 만약 페이지 크기가 다른 부분에서 정의되어 있다면, 해당 변수를 적절히 주입하거나 설정해줄 필요가 있습니다. 그렇지 않다면, 해당 변수를 추가하고 명확하게 정의해야 합니다. -
예외 처리:
getListOfCompanies()메서드에서companyRepository.findPage(pageable)호출 시 예외가 발생할 경우 어떻게 처리할지에 대한 전략이 없습니다. 예외 처리를 통해 사용자가 알 수 있도록 하거나 로그를 남기는 등의 처리가 필요합니다. -
코드 가독성: 메서드 이름이나 클래스 이름, 변수 이름 등을 좀 더 명확하게 지음으로써 코드 가독성을 높일 수 있습니다. 예를 들어,
getListOfCompanies보다는fetchCompaniesByPage와 같은 이름이 더 직관적일 수 있습니다.
이 외에 추가적인 개선 사항이 있을 수 있으니, 프로젝트의 전체 구조와 요구사항도 고려하여 검토하시기 바랍니다.
바로 MERGE하지 마세요! 검토 후 MERGE해야합니다!
✅ 최근 작업 주제 (하나 이상의 주제를 선택해주세요.)
🏆 구현 목표
회사 등록 및 조회
📋 구현 사항 설명 (작업한 내용을 상세하게 기록합니다.)
🔍 테스트 방법 (변경 사항을 확인하기 위한 테스트 방법을 기술합니다.)
Postman으로 API 호출 테스트
🛠️ 쓰이는 모듈
💬 기타 질문 및 특이 사항