Skip to content

Refactor/put project #297

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.

Sign up for GitHub

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

Closed
Eseas wants to merge 3 commits into from
Closed

Refactor/put project #297

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
2b6e0b3
refactor : project modify company id can't modify
Eseas Feb 19, 2025
294f27e
refactor : project modify company id can't modify
Eseas Feb 19, 2025
b05b3b6
refactor : put project fix
Eseas Feb 19, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion Admin/src/main/java/com/seveneleven/project/dto/PatchAuthorization.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
import com.seveneleven.entity.member.Member;
import com.seveneleven.entity.project.Project;
import com.seveneleven.entity.project.ProjectAuthorization;
import com.seveneleven.entity.project.constant.Authorization;
import com.seveneleven.entity.project.constant.MemberType;
import lombok.Getter;
import lombok.NoArgsConstructor;
Expand All @@ -19,7 +20,7 @@ public static abstract class BaseMemberAuthorization implements MemberAuthorizat
protected Long memberId;
protected String memberName;
protected MemberType memberType;
protected String projectAuthorization;
protected Authorization projectAuthorization;

public abstract ProjectAuthorization toEntity(Project project, Member member);

Copy link

@github-actions github-actions bot Feb 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

코드 패치에 대한 간단한 리뷰를 아래에 제시합니다.

코드 리뷰

  1. 타입 변경:

    • projectAuthorization의 타입을 String에서 Authorization으로 변경했습니다.
    • 이 변경은 명확성과 타입 안전성을 높이는 긍정적인 개선입니다. String으로 표현되는 권한보다 Authorization 객체를 사용하면 코드의 가독성과 유지보수성이 향상됩니다.

  2. 의존성:

    • Authorization 클래스를 추가함으로써, 해당 클래스에 대한 의존성이 생깁니다.
    • 클래스가 잘 정의되어 있고 필요한 모든 메서드와 속성을 포함하고 있는지 확인해야 합니다. 아니면 코드에 오류가 발생할 가능성이 있습니다.

  3. 추상 메서드:

    • toEntity 메서드가 잘 정의되어 있습니다. 하지만 이 메서드의 구현체에서 Authorization 타입을 어떻게 처리할 것인지에 대한 고려가 필요합니다.
    • 이를 위해 추가적인 검증 로직이 필요할 수 있습니다.

  4. Null 안전성:

    • projectAuthorization을 Authorization 타입으로 변경하면서, null 값이 될 수 있는 가능성에 대해 확인이 필요합니다.
    • null 체크 또는 기본값 설정을 고려해보세요.

  5. 주석 및 문서화:

    • 이 변경 사항에 대한 주석이나 문서화가 없으므로, 코드의 가독성을 높이기 위해 해당 필드의 목적과 사용 방법에 대한 설명을 추가하는 것이 좋습니다.

결론

변경된 코드는 명확성과 타입 안전성을 높이는 긍정적인 방향으로 개선되었습니다. 하지만 의존성 관리와 null 안전성, 및 문서화에 추가적인 주의가 필요합니다. 코드베이스에 통합하기 전에 충분한 테스트를 수행하는 것이 좋습니다.

Expand Down
4 changes: 0 additions & 4 deletions Admin/src/main/java/com/seveneleven/project/dto/PutProject.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,6 @@ public static class Request {
private Long projectTypeId; // 프로젝트 유형 ID
private ProjectStatusCode projectStatusCode; //
private String bnsManager; // BNS 담당자 ID (Member 엔티티의 ID)
private String contractNumber; // 계약서 번호
private LocalDate plannedStartDate; // 시작 예정일
private LocalDate plannedEndDate; // 종료 예정일
private LocalDate startDate; // 시작일
Expand All @@ -54,7 +53,6 @@ public Project updateProject(
projectName,
projectDescription,
projectStatusCode,
contractNumber,
plannedStartDate,
plannedEndDate,
startDate,
Expand All @@ -76,7 +74,6 @@ public static class Response {
private String projectType; // 프로젝트 유형 ID
private ProjectStatusCode projectStatusCode; //
private String bnsManager; // BNS 담당자 ID (Member 엔티티의 ID)
private String contractNumber; // 계약서 번호
private LocalDate plannedStartDate; // 시작 예정일
private LocalDate plannedEndDate; // 종료 예정일
private LocalDate startDate; // 시작일
Expand All @@ -101,7 +98,6 @@ private Response(Project project, List<ProjectTag> tags, List<ProjectAuthorizati
projectDescription = project.getProjectDescription();
projectType = project.getProjectType().getProjectTypeName();
bnsManager = project.getBnsManager();
contractNumber = project.getContractNumber();
plannedStartDate = project.getPlannedStartDate();
plannedEndDate = project.getPlannedEndDate();
startDate = project.getStartDate(); // 시작일
Copy link

@github-actions github-actions bot Feb 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

코드 패치에 대한 간단한 리뷰를 진행하겠습니다.

  1. 변수 제거: contractNumber 변수가 몇 군데에서 제거되었습니다. 이 부분이 의도적인 것인지 확인이 필요합니다. 만약 contractNumber가 더 이상 필요 없다면, 관련 로직에서 이 값을 참조하고 있는 부분이 없도록 전체적으로 점검해야 합니다.

  2. 유지보수성: 만약 contractNumber 관련 기능이 실제로 제거되었다면, 이와 관련된 주석이나 문서화도 함께 업데이트 하는 것이 좋습니다. 코드 내에서 제거된 사항에 대한 설명이 없으면 나중에 코드를 읽는 사람이 혼란스러워 할 수 있습니다.

  3. Nullable 처리: contractNumber가 제거되면, 경우에 따라서 관련된 데이터베이스 필드나 API 응답에서도 이 필드의 존재 여부를 확인할 필요가 있습니다. 데이터베이스 스키마나 기존의 API 문서도 점검하여 일관성을 유지하는 것이 중요합니다.

  4. 테스트 케이스: 이 변경 사항으로 인해 기존의 테스트 케이스가 실패할 수 있습니다. contractNumber와 관련된 모든 테스트가 포함되어 있는지 확인하고, 필요 없어진 테스트는 삭제하고, 새로운 로직에 대한 테스트를 추가해야 합니다.

  5. 의존성 문제: 만약 contractNumber가 다른 클래스나 메서드에서 사용되었다면, 이들을 찾아서 어떤 영향을 미칠지 분석해야 합니다. 원치 않는 런타임 에러가 발생할 수 있습니다.

  6. 데이터 무결성: 계약서 번호는 비즈니스 로직에서 중요한 역할을 할 수 있습니다. 제거한 이유가 비즈니스 요구사항 변화 때문이라면, 그러한 변화를 문서화하거나 관련 이해관계자와 논의하는 것이 좋습니다.

이러한 점들을 고려하여 코드를 점검하고, 필요시 수정 및 추가 작업을 수행하는 것이 좋습니다. 전체적인 의도와 비즈니스 요구사항에 부합하는 방향으로 코드를 유지보수하는 것이 중요합니다.

Expand Down
2 changes: 2 additions & 0 deletions Admin/src/main/java/com/seveneleven/project/repository/AdminProjectTagRepository.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,4 +10,6 @@
@Repository
public interface AdminProjectTagRepository extends JpaRepository<ProjectTag, Long> {
List<ProjectTag> findByProjectIdAndIsActive(Long projectId, YesNo isActive);


}
Copy link

@github-actions github-actions bot Feb 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

코드 패치를 검토해본 결과, 다음과 같은 사항들을 지적할 수 있습니다:

  1. 여분의 줄: 두 개의 빈 줄이 추가되었습니다. 코드의 가독성을 높이기 위해서 불필요한 공백은 피하는 것이 좋습니다. 일반적으로 두 줄 이상의 빈 줄은 권장되지 않습니다.

  2. 이름 명확성: 메서드 이름 findByProjectIdAndIsActive는 괜찮지만, YesNo 열거형으로 사용되고 있는 인자에 대해 주석을 추가하면 좋습니다. 이 열거형이 '예/아니오' 외 다른 값을 가질 경우, 호출하는 쪽에서 혼란이 발생할 수 있습니다.

  3. 예외 처리: 이 리포지토리는 데이터베이스와의 연결에 문제가 생길 경우 적절한 예외 처리가 필요할 수 있습니다. 현재 상태로는 예외 처리 로직이 보이지 않으므로, 서비스 레이어에서 적절한 예외 처리를 추가해야 할 필요가 있습니다.

  4. Query Method의 성능: 데이터베이스에서 데이터를 조회할 때, find 메서드가 많은 데이터를 반환할 가능성이 있는 경우 페이징과 정렬을 고려해야 할 수 있습니다. 필요하다면 Pageable 객체를 도입해 보세요.

  5. 테스트 추가: 새로운 메서드가 추가되기 때문에, 유닛 테스트도 함께 작성하여 해당 메서드의 동작을 검증하는 것이 좋습니다.

이러한 사항들을 고려하여 코드를 개선하면, 더욱 안정적이고 유지보수하기 쉬운 코드가 될 것입니다.

5 changes: 3 additions & 2 deletions Admin/src/main/java/com/seveneleven/project/service/AdminProjectTagServiceImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@
public class AdminProjectTagServiceImpl implements AdminProjectTagService {

private final ProjectTagReader projectTagReader;
private final AdminProjectTagStore adminProjectTagStore;
private final AdminProjectTagStore projectTagStore;

@Override
public List<ProjectTag> getAllProjectTags(Long projectId) {
Expand All @@ -21,6 +21,7 @@ public List<ProjectTag> getAllProjectTags(Long projectId) {

@Override
public List<ProjectTag> storeProjectTags(Project project, List<String> tags) {
return adminProjectTagStore.store(project, tags);
List<ProjectTag> projectTags = projectTagReader.readByProjectId(project.getId());
return projectTagStore.store(project, tags, projectTags);
}
}
Copy link

@github-actions github-actions bot Feb 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

코드 패치를 검토해 보니 몇 가지 개선 사항과 잠재적인 버그 위험이 보입니다.

  1. 변경된 변수 이름:

    • adminProjectTagStore에서 projectTagStore로 이름이 변경되었습니다. 이 변경이 기존의 코드와 잘 통합되는지 확인해야 합니다. 특히, 사용되는 클래스의 다른 부분에서도 이 변경된 이름을 반영하고 있는지 점검할 필요가 있습니다.

  2. 메소드 조건 확인:

    • storeProjectTags 메소드에서 projectTagReader.readByProjectId(project.getId())를 호출하여 프로젝트 태그를 가져오고 있습니다. 이 메소드가 반환하는 결과가 항상 null이 아닌지 확인하는 것이 중요합니다. 예를 들어, 비어 있는 결과가 반환될 경우 projectTagStore.store 메소드는 제대로 작동할지 의문입니다.

  3. 오류 처리:

    • 만약 projectTagReader.readByProjectId에서 예외가 발생한다면 이를 적절히 처리하지 않으면 어플리케이션이 크래시할 수 있습니다. 예외 처리를 추가하여 코드의 안정성을 높이는 것이 좋을 것 같습니다.

  4. 입력 검증:

    • storeProjectTags 메소드의 매개변수인 project와 tags에 대한 검증이 필요합니다. null 또는 유효하지 않은 값이 전달될 경우 이를 처리하는 로직을 추가하는 것이 좋습니다.

  5. 주석 추가:

    • 메소드와 클래스에 대한 주석이 없으므로, 코드의 의도를 이해하기 어렵습니다. 각 메소드가 수행하는 일이 무엇인지 간단한 주석을 추가하는 것이 유지 보수성을 높이는 데 도움이 됩니다.

  6. 테스트 추가:

    • 변경된 코드에 대한 단위 테스트가 있다면, 이 테스트가 모든 시나리오를 포괄하고 있는지 확인해야 합니다. 새로운 로직이 추가되었으므로 관련된 테스트 케이스를 작성하는 것이 중요합니다.

이 외에도 코드 스타일이나 일관성 같은 점을 검토할 수 있지만, 주된 리스크와 개선 사항은 위와 같습니다.

2 changes: 1 addition & 1 deletion Admin/src/main/java/com/seveneleven/project/service/AdminProjectTagStore.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,5 +6,5 @@
import java.util.List;

public interface AdminProjectTagStore {
List<ProjectTag> store(Project project, List<String> tags);
List<ProjectTag> store(Project project, List<String> tags, List<ProjectTag> projectTags);
}
Copy link

@github-actions github-actions bot Feb 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

여기 제시된 코드 패치에 대한 간단한 코드 리뷰입니다.

  1. 변경 사항 검토:

    • store 메서드의 파라미터에 List<ProjectTag> projectTags가 추가되었습니다. 이 변경은 메서드가 더 많은 정보를 처리할 수 있도록 확장된 것입니다.

  2. 버그 위험:

    • projectTags 인자에 대한 유효성 검사가 필요합니다. 이 리스트가 null일 경우, 또는 예상치 못한 데이터가 전달될 경우 예외가 발생할 수 있습니다. 적절한 예외 처리 로직을 추가하는 것이 좋습니다.

  3. 코드 개선 제안:

    • 메서드의 의미를 더욱 명확하게 하기 위해 Javadoc 주석을 추가하는 것을 추천합니다. 파라미터에 대한 설명과 메서드의 기능을 서술하면 코드의 가독성이 향상됩니다.
    • 또한, List<String> tags와 List<ProjectTag> projectTags가 서로 어떻게 관계되는지에 대한 설명이 필요할 수 있습니다. 가능하다면 이를 명확히 문서화해야 합니다.

  4. 테스트 검증:

    • 변경된 store 메서드에 대한 테스트 케이스를 추가하여 새로운 파라미터에 대한 시나리오를 검증해야 합니다. 특히, 빈 리스트, null 값, 그리고 예상치 못한 입력값에 대한 테스트가 중요합니다.

이 부분들을 고려하여 코드를 개선하면 더욱 안정적이고 유지보수하기 쉬운 코드를 만들 수 있을 것입니다.

7 changes: 5 additions & 2 deletions Admin/src/main/java/com/seveneleven/project/service/AdminProjectTagStoreImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,11 +15,14 @@ public class AdminProjectTagStoreImpl implements AdminProjectTagStore {
private final AdminProjectTagRepository projectTagRepository;

@Override
public List<ProjectTag> store(Project project, List<String> tagNameList) {
public List<ProjectTag> store(Project project, List<String> tagNameList, List<ProjectTag> projectTags) {
if(tagNameList == null || tagNameList.isEmpty()) {
return List.of();
return projectTags;
}

// TODO - 리스트 확인 후 추가 및 삭제 처리
projectTagRepository.deleteAll(projectTags);

List<ProjectTag> ProjectTags = tagNameList
.stream()
.map(tagName -> ProjectTag.create(project, tagName))
Copy link

@github-actions github-actions bot Feb 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

코드 리뷰 결과입니다:

  1. 인수 추가: store 메소드에 List<ProjectTag> projectTags 인수를 추가한 것은 유용해 보입니다. 이것은 호출하는 쪽에서 삭제할 태그 목록을 명시적으로 전달하여 코드의 명확성을 증가시킵니다.

  2. Null 체크: tagNameList에 대한 null 체크를 포함한 점은 좋습니다. 다만, projectTags에 대해서도 null 체크를 추가하는 것이 좋습니다. 예를 들어, projectTags가 null인 경우에는 어떻게 처리할지에 대한 로직이 필요할 수 있습니다.

  3. 리스트 확인 및 삭제 처리: 주석으로 남겨진 "리스트 확인 후 추가 및 삭제 처리" 부분이 중요한 것 같습니다. 삭제할 태그를 명확하게 정의할 필요가 있으며, 새 태그가 추가될 때 현재 저장된 태그와 어떤 비교를 할지 구체적으로 구현하는 것이 좋습니다. 현재 deleteAll(projectTags); 호출은 프로젝트 태그가 없는 경우에 오류를 발생시킬 수 있습니다.

  4. 로깅 부족: 태그 삭제 및 추가 시 어떤 작업이 수행되었는지에 대한 로깅이 없으므로, 향후 디버깅을 위해 로깅을 추가하는 것이 좋습니다.

  5. 변수 명: ProjectTags는 일반적인 규칙에 따라 소문자로 시작하는 것이 좋습니다. 예를 들어, projectTags로 이름을 변경하는 것을 권장합니다.

  6. 예외 처리: 저장이나 삭제 과정에서 예외가 발생할 수 있으므로, 해당 부분에 대한 예외 처리가 필요합니다. 이를 통해 시스템의 안정성을 높일 수 있습니다.

이러한 점들을 고려하여 코드를 리팩토링하면 안정성과 유지보수성을 높일 수 있을 것입니다.

Expand Down
12 changes: 0 additions & 12 deletions Common/src/main/java/com/seveneleven/Main.java
View file
Open in desktop

This file was deleted.

2 changes: 0 additions & 2 deletions Common/src/main/java/com/seveneleven/entity/project/Project.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -118,7 +118,6 @@ public Project update(
String name,
String description,
ProjectStatusCode statusCode,
String contractNumber,
LocalDate plannedStartDate,
LocalDate plannedEndDate,
LocalDate startDate,
Expand All @@ -132,7 +131,6 @@ public Project update(
this.projectType = ProjectType;
this.projectStatusCode = statusCode;
this.bnsManager = bnsManager;
this.contractNumber = contractNumber;
this.plannedStartDate = plannedStartDate;
this.plannedEndDate = plannedEndDate;
this.startDate = startDate;
Copy link

@github-actions github-actions bot Feb 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

코드 리뷰를 진행하겠습니다.

  1. 불필요한 변수 제거: contractNumber 변수가 코드에서 삭제되었습니다. 만약 이 변수가 더 이상 필요하지 않다면 괜찮지만, 프로젝트의 비즈니스 로직에 따라 중요할 수 있으니 삭제 이유에 대한 문서화가 필요합니다.

  2. 주석 처리: 이변경에 대한 주석이 없습니다. 다른 개발자들이 코드를 이해하기 쉽게 이유를 명시한 주석을 추가하는 것이 좋습니다.

  3. 변수 초기화: 삭제된 contractNumber 변수와 같은 변수가 이 클래스에서 사용되고 있던 다른 메서드나 내부 로직이 없어진 것인지 확인이 필요합니다. 만약 이 변수가 다른 메서드에서 사용되고 있었다면, 이 변경으로 인해 NullPointerException 등의 버그가 발생할 수 있습니다.

  4. 테스트 케이스 검토: 해당 변경 사항에 대해 적절한 테스트가 이루어졌는지 확인해야 합니다. contractNumber가 관련된 테스트가 빠져 있다면, 해당 부분에 대한 테스트를 추가하는 것이 필요합니다.

  5. 변수 명명 규칙: ProjectType 변수명이 대문자로 시작하는데, 이는 일반적인 자바 변수명 규칙에 어긋납니다. 변수를 소문자로 시작하도록 수정하는 것이 좋습니다 (예: projectType).

전반적으로 주요 기능에 대한 영향을 미칠 수 있는 변경이므로, 삭제 이력과 관련된 문서화 및 충분한 테스트가 이루어졌는지 확인하는 것이 중요합니다. 추가적으로 다른 부분에서 문제가 발생하지 않도록 전체 코드베이스에 대한 검토를 해보는 것도 좋습니다.

Expand Down
14 changes: 10 additions & 4 deletions Common/src/main/java/com/seveneleven/entity/project/ProjectAuthorization.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
import com.seveneleven.entity.global.YesNo;
import com.seveneleven.entity.global.converter.YesNoConverter;
import com.seveneleven.entity.member.Member;
import com.seveneleven.entity.project.constant.Authorization;
import com.seveneleven.entity.project.constant.MemberType;
import com.seveneleven.entity.project.duplkey.MemberProjectStepId;
import jakarta.persistence.*;
Expand Down Expand Up @@ -34,12 +35,13 @@ public class ProjectAuthorization extends BaseEntity {
private MemberType memberType; // 회원 구분 (client, developer)

// Question - Enum으로 뺄까?
private String authorizationCode; // 권한 코드
@Enumerated(EnumType.STRING)
private Authorization authorizationCode; // 권한 코드

@Convert(converter = YesNoConverter.class)
private YesNo isActive; // 삭제 여부

private ProjectAuthorization(Member member, Project project, MemberType memberType, String authorizationCode) {
private ProjectAuthorization(Member member, Project project, MemberType memberType, Authorization authorizationCode) {
this.id = new MemberProjectStepId(member.getId(), project.getId());
this.member = member;
this.project = project;
Expand All @@ -48,11 +50,11 @@ private ProjectAuthorization(Member member, Project project, MemberType memberTy
this.isActive = YesNo.YES;
}

public static ProjectAuthorization create(Member member, Project project, MemberType memberType, String authorizationCode) {
public static ProjectAuthorization create(Member member, Project project, MemberType memberType, Authorization authorizationCode) {
return new ProjectAuthorization(member, project, memberType, authorizationCode);
}

public void edit(MemberType memberType, String authorizationCode) {
public void edit(MemberType memberType, Authorization authorizationCode) {
this.memberType = memberType;
this.authorizationCode = authorizationCode;
}
Expand All @@ -61,4 +63,8 @@ public ProjectAuthorizationHistory delete() {
isActive = YesNo.NO;
return ProjectAuthorizationHistory.create(this);
}

public String getAuthorization() {
return authorizationCode.name();
}
}
Copy link

@github-actions github-actions bot Feb 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

코드 패치를 검토해본 결과, 다음과 같은 몇 가지 사항을 지적할 수 있습니다:

  1. Enums 사용의 장점: authorizationCode 필드를 String에서 Authorization enum으로 변경한 것은 좋은 결정입니다. 이는 코드의 가독성을 높이고, 잘못된 값의 입력을 방지하는 데 도움이 됩니다.

  2. 정상적인 메서드 시그니처 변경: create 및 edit 메서드의 시그니처를 변경하여 String 대신 Authorization을 사용한 것도 잘 되어 있습니다. 이렇게 하면 코드의 일관성이 유지되고, 유지보수성이 높아집니다.

  3. getAuthorization 메서드: getAuthorization 메서드를 추가하여 authorizationCode의 이름을 반환하는 것은 좋은 접근이지만, authorizationCode가 null일 경우 NullPointerException을 피하기 위한 처리가 필요할 수 있습니다. 예를 들어:

    public String getAuthorization() {
    return authorizationCode != null ? authorizationCode.name() : null;
}

    또는 Optional을 사용하는 방법도 있습니다.

  4. 주석 추가: 코드에 주석이 적혀 있지만, authorizationCode 필드가 Enum으로 사용되는 이유나 getAuthorization 메서드의 목적에 대해 추가적인 주석을 추가하는 것이 좋습니다. 이는 다른 개발자 또는 나중에 코드를 읽는 사람이 이해하는 데 도움이 됩니다.

  5. 예외 처리: create와 edit 메서드에 대한 입력값 검증이 필요할 수 있습니다. 회원이나 프로젝트가 null인 경우 예외를 던지는 것이 좋습니다.

  6. 시맨틱 버전 관리: 이와 같은 드라마틱한 변경 사항은 버전 관리를 통해 관리하는 것이 좋습니다. 기존 코드를 사용하는 클라이언트가 예기치 않게 영향을 받을 수 있으므로, 변경된 사항에 대한 문서를 작성하는 것도 도움이 될 것입니다.

이상으로 코드 리뷰를 마칩니다. 이 패치는 전반적으로 잘 되어 있으나, 몇 가지 개선 사항을 통해 더욱 견고한 코드를 만들 수 있을 것 같습니다.

2 changes: 1 addition & 1 deletion Common/src/main/java/com/seveneleven/entity/project/ProjectAuthorizationHistory.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ private ProjectAuthorizationHistory(ProjectAuthorization authorization) {
this.memberName = authorization.getMember().getName();
this.projectName = authorization.getProject().getProjectName();
this.memberType = authorization.getMemberType().name();
this.authorizationCode = authorization.getAuthorizationCode();
this.authorizationCode = authorization.getAuthorization();
this.isActive = authorization.getIsActive();
}

Copy link

@github-actions github-actions bot Feb 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

코드 패치에 대한 간단한 리뷰를 진행하겠습니다.

  1. 변경 사항 검사:

    • authorization.getAuthorizationCode()를 authorization.getAuthorization()으로 변경했습니다. 이 변경은 코드의 기능적 동작에 영향을 미칠 수 있으므로, getAuthorization() 메소드가 getAuthorizationCode()와 동일한 데이터 타입과 의미를 가지는지 확인해야 합니다. 그렇지 않다면, 이로 인해 버그가 발생할 수 있습니다.

  2. 코드 가독성:

    • this.authorizationCode라는 변수의 이름이 의미하는 바가 명확해야 하므로, getAuthorization() 메소드의 본문을 확인하여 이 메소드가 어떤 값을 반환하는지 명확히 이해할 필요가 있습니다.

  3. 에러 핸들링:

    • authorization 객체가 null일 경우 NullPointerException이 발생할 수 있습니다. 이 경우를 처리하기 위한 정부 처리가 필요합니다.

  4. 주석 추가:

    • 코드 변경의 이유를 설명하는 주석을 추가하는 것이 좋습니다. 특히, getAuthorization으로의 변경 이유는 코드의 유지보수성을 높이는 데 도움이 됩니다.

  5. 단위 테스트:

    • 해당 변경에 대한 단위 테스트가 있는지 확인하고, 없다면 추가하는 것이 좋습니다. 특히 변경된 메소드에 대한 테스트를 통해 예기치 못한 버그를 방지할 수 있습니다.

위의 사항들을 고려하여 코드를 보다 안전하고 가독성 있게 개선할 수 있을 것입니다.

Expand Down
2 changes: 1 addition & 1 deletion Main/src/main/java/com/seveneleven/project/dto/GetMemberAuthorization.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ public String toString() {

private Response(ProjectAuthorization authorization) {
this.memberId = authorization.getMember().getId();
this.authorization = authorization.getAuthorizationCode();
this.authorization = authorization.getAuthorization();
this.memberType = authorization.getMemberType();
}

Copy link

@github-actions github-actions bot Feb 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

코드 패치에 대한 간단한 리뷰를 제공하겠습니다.

  1. 변경 내용: authorization.getAuthorizationCode()에서 authorization.getAuthorization()으로 변경되었습니다. 이 변경이 의도된 대로 작동하는지, getAuthorization() 메서드가 올바른 형식의 값을 반환하는지 확인해야 합니다. 기존 코드에서 깃발이 될 수 있는 점은, getAuthorizationCode()와 getAuthorization()의 반환 값이 동일한 형식이 아닐 수 있다는 것입니다.

  2. 명확성: authorization이라는 이름은 다소 애매할 수 있습니다. getAuthorization()이 반환하는 값이 어떤 의미인지 명확하지 않다면, 더 구체적인 이름으로 변경하는 것이 코드 가독성을 높일 수 있습니다. 예를 들어, getAuthorizationDetails()와 같은 이름으로 변경하는 것이 좋을 수 있습니다.

  3. 예외 처리: authorization.getAuthorization() 호출 시 NullPointerException이 발생할 수 있습니다. authorization 객체가 null이 아닌지, 혹은 getAuthorization() 메서드가 null을 반환할 가능성이 있는지 확인하고, 이에 대한 예외 처리를 추가하는 것이 좋습니다.

  4. 주석 추가: 변경된 코드의 의도를 설명하는 주석을 추가하는 것도 가독성을 높일 수 있습니다. 이는 다른 개발자가 코드를 이해하는 데 도움이 될 것입니다.

  5. 테스트 케이스: 변경 사항이 기존 기능에 영향을 미칠 수 있으므로, 관련된 테스트 케이스를 작성하거나 기존 테스트가 통과하는지 확인해야 합니다. 특히 이 부분이 중요한 이유는 getAuthorization()의 반환 값이 기존과는 다를 수 있기 때문입니다.

이러한 점들을 고려하여 코드를 개선할 수 있습니다. 전반적으로, 변경 내용이 성공적으로 의도한 바를 달성하는지 검토하고, 가능한 예외 상황에 대한 처리가 필요할 것 같습니다.

Expand Down
4 changes: 2 additions & 2 deletions Main/src/main/java/com/seveneleven/project/dto/GetProjectAuthorization.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@ public String toString() {
private CustomerMemberAuthorization(ProjectAuthorization projectAuthorization) {
this.memberId = projectAuthorization.getMember().getId();
this.memberName = projectAuthorization.getMember().getName();
this.projectAuthorization = projectAuthorization.getAuthorizationCode();
this.projectAuthorization = projectAuthorization.getAuthorization();
}

public static CustomerMemberAuthorization toDto(ProjectAuthorization projectAuthorization) {
Expand All @@ -82,7 +82,7 @@ public String toString() {
private DeveloperMemberAuthorization(ProjectAuthorization projectAuthorization) {
this.memberId = projectAuthorization.getMember().getId();
this.memberName = projectAuthorization.getMember().getName();
this.projectAuthorization = projectAuthorization.getAuthorizationCode();
this.projectAuthorization = projectAuthorization.getAuthorization();
}

public static DeveloperMemberAuthorization toDto(ProjectAuthorization projectAuthorization) {
Copy link

@github-actions github-actions bot Feb 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

코드 패치에 대한 간단한 리뷰를 제공하겠습니다.

  1. 변경 내용: projectAuthorization.getAuthorizationCode()를 projectAuthorization.getAuthorization()으로 변경한 것으로 보입니다. 이는 메서드 이름이 명확해지면서 더 나은 의미를 가질 가능성이 있습니다.

  2. 버그 리스크:

    • 만약 getAuthorization() 메서드가 null을 반환할 수 있다면, projectAuthorization 필드가 null이 되는 경우를 처리해야 합니다. 이렇게 되면 후속 코드에서 NullPointerException이 발생할 수 있습니다.
    • 또한, getAuthorizationCode()에서 getAuthorization()으로의 변경으로 인해 의도하지 않은 기능상의 변화가 있을 수 있습니다. 이 메서드들 간의 차이점을 잘 이해하고 있는지 확인할 필요가 있습니다.

  3. 개선 제안:

    • 메서드 변경에 대한 테스트 케이스를 추가하는 것이 좋습니다. 특히, 변경된 메서드가 예상대로 작동하는지 확인하기 위한 단위 테스트를 포함해야 합니다.
    • 문서화: 해당 변화에 대한 문서나 주석을 추가하여 이후 코드 유지보수 시에 혼선을 방지할 수 있습니다.
    • 변수 검사: getAuthorization()의 반환값을 확인 후 초기화하는 방법을 사용하는 것이 좋습니다. 예를 들어, this.projectAuthorization = projectAuthorization.getAuthorization() != null ? projectAuthorization.getAuthorization() : DEFAULT_VALUE;와 같은 방식입니다.

  4. 전반적인 평가: 전반적으로 메서드 이름이 더 명확해지는 긍정적인 변화이지만, 변경으로 인한 잠재적인 문제를 고려하고 철저한 검증이 필요합니다. 변경 사항을 빠르고 안전하게 반영하기 위해 추가적인 테스트를 권장합니다.

Expand Down
4 changes: 2 additions & 2 deletions Main/src/main/java/com/seveneleven/project/dto/GetProjectDetail.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -110,7 +110,7 @@ private CustomerMemberAuthorization(ProjectAuthorization projectAuthorization) {
this.memberName = projectAuthorization.getMember().getName();
this.department = projectAuthorization.getMember().getDepartment();
this.position = projectAuthorization.getMember().getPosition();
this.projectAuthorization = projectAuthorization.getAuthorizationCode();
this.projectAuthorization = projectAuthorization.getAuthorization();
}

public static CustomerMemberAuthorization toDto(ProjectAuthorization projectAuthorization) {
Expand Down Expand Up @@ -138,7 +138,7 @@ private DeveloperMemberAuthorization(ProjectAuthorization projectAuthorization)
this.memberName = projectAuthorization.getMember().getName();
this.department = projectAuthorization.getMember().getDepartment();
this.position = projectAuthorization.getMember().getPosition();
this.projectAuthorization = projectAuthorization.getAuthorizationCode();
this.projectAuthorization = projectAuthorization.getAuthorization();
}

public static DeveloperMemberAuthorization toDto(ProjectAuthorization projectAuthorization) {
Copy link

@github-actions github-actions bot Feb 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

코드 패치를 검토한 결과, 몇 가지 주의할 점과 개선 사항을 제안할 수 있습니다.

  1. 메서드 이름 변화: getAuthorizationCode()에서 getAuthorization()으로 변경되었습니다. 이 변경이 의도하는 바는 명확해 보이지만, 호출하는 다른 코드에서 메서드 이름 변경으로 인한 오류가 발생할 수 있습니다. 이 메서드를 호출하는 모든 곳에서 변경 사항이 반영되었는지 확인해야 합니다.

  2. 주석 추가: 변경된 메서드의 목적이나 의도를 명확히 하기 위해 주석을 추가하는 것이 좋습니다. 이를 통해 코드의 가독성이 향상되고, 다른 개발자들이 코드를 이해하는 데 도움이 됩니다.

  3. 예외 처리: projectAuthorization이 null일 경우를 고려해야 합니다. 현재 코드는 null 체크가 없는 상태입니다. null 값이 넘어올 경우 NullPointerException이 발생할 수 있으므로, 이를 처리하는 로직을 추가하는 것이 좋습니다.

  4. 일관성 유지: CustomerMemberAuthorization과 DeveloperMemberAuthorization에서 동일한 변경이 적용되었으니, 다른 관련 클래스에서도 같은 메서드 이름 변경이 필요한지 확인해야 합니다.

  5. 코드 포맷팅: 코드의 일관된 포맷팅을 유지하여 가독성을 높이는 것이 좋습니다. 예를 들어, 클래스/메서드 사이에 적절한 빈 줄을 추가하면 더 깔끔하게 보일 수 있습니다.

요약하면, 메서드 이름 변경으로 인한 링크 끊김, null 검사 추가, 주석 및 포맷팅 개선 등을 통해 코드를 안정적이고 가독성 좋게 만들 수 있습니다.

Expand Down
5 changes: 3 additions & 2 deletions Main/src/main/java/com/seveneleven/project/dto/PostProjectAuthorization.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
import com.seveneleven.entity.member.Member;
import com.seveneleven.entity.project.Project;
import com.seveneleven.entity.project.ProjectAuthorization;
import com.seveneleven.entity.project.constant.Authorization;
import com.seveneleven.entity.project.constant.MemberType;
import lombok.Getter;
import lombok.NoArgsConstructor;
Expand Down Expand Up @@ -30,7 +31,7 @@ public interface MemberAuthorization {
public static abstract class BaseMemberAuthorization implements MemberAuthorization {
protected Long memberId;
protected MemberType memberType;
protected String projectAuthorization;
protected Authorization projectAuthorization;

public abstract ProjectAuthorization toEntity(Project project, Member member);

Expand Down Expand Up @@ -92,7 +93,7 @@ public static Response create(Long projectId) {
public static class FailList {
private Long memberId;
private MemberType memberType;
private String projectAuthorization;
private Authorization projectAuthorization;
private HttpStatus status;
private String message;

Copy link

@github-actions github-actions bot Feb 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

코드 패치에 대한 간단한 리뷰를 제공하겠습니다. 아래는 발견된 버그 위험 요소와 개선 제안입니다:

  1. 타입 변경에 따른 위험: String projectAuthorization에서 Authorization projectAuthorization으로 변경되었습니다. 이로 인해 코드의 다른 부분에서 projectAuthorization을 사용하는 부분에서 타입 불일치가 발생할 수 있습니다. 이를 확인해야 합니다.

  2. Null 안전성: Authorization 타입이 nullable할 수 있는 상황에서, 이를 사용하는 부분에서 null 체크를 하지 않으면 NullPointerException이 발생할 수 있습니다. projectAuthorization에 대한 null 체크를 추가하는 것이 좋습니다.

  3. 클래스 및 인터페이스 문서화: MemberAuthorization 인터페이스와 BaseMemberAuthorization 클래스에 대한 JavaDoc을 추가하는 것이 좋습니다. 각 메서드와 클래스의 역할을 명확히 문서화하여 코드의 가독성과 유지보수성을 향상시킬 수 있습니다.

  4. 상수 사용: Authorization 클래스의 사용이 늘어날 경우, 이와 관련된 상수를 잘 정의하여 사용하는 것이 좋습니다. 공간을 절약하고 코드의 가독성을 높일 수 있습니다.

  5. Error Handling: create 메서드의 책임에 대한 예외 처리 로직이 부족한 것 같습니다. 메서드 사용시 발생할 수 있는 오류에 대한 처리(예: 프로젝트가 존재하지 않거나 잘못된 멤버 ID 등이 들어오는 경우)를 고려해야 합니다.

  6. Testing: 변경 사항이 기존 기능에 영향을 미치지 않도록 단위 테스트가 필요합니다. 특히, 타입 변경으로 인한 코드 경로의 변화를 테스트하는 것이 중요합니다.

이러한 사항들을 고려하여 코드를 수정하면 좀 더 안정적이고 유지보수하기 쉬운 코드가 될 것입니다.

Expand Down
Loading