Skip to content

refactor : project steps return index #295

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

Merged
Eseas merged 3 commits into from
Feb 19, 2025
Merged

refactor : project steps return index #295

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
c926e0d
refactor : project steps return index
Eseas Feb 18, 2025
3865e79
refactor : project steps return index fix
Eseas Feb 18, 2025
9808c4b
refactor : project modify company id can't modify
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
5 changes: 0 additions & 5 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
@@ -1,6 +1,5 @@
package com.seveneleven.project.dto;

import com.seveneleven.entity.member.Company;
import com.seveneleven.entity.project.Project;
import com.seveneleven.entity.project.ProjectAuthorization;
import com.seveneleven.entity.project.ProjectTag;
Expand Down Expand Up @@ -49,8 +48,6 @@ public String toString() {

public Project updateProject(
Project project,
Company customer,
Company developer,
ProjectType projectType
) {
return project.update(
Expand All @@ -63,8 +60,6 @@ public Project updateProject(
startDate,
endDate,
finalApprovalDate,
customer,
developer,
projectType,
bnsManager
);
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. 삭제된 매개변수 확인: Company customer와 Company developer 매개변수가 삭제되었습니다. 이 두 개체가 update 메서드에서 필요하지 않다면, 메서드에 대한 호출에서도 이 매개변수들이 제거되었는지 확인이 필요합니다. 그렇지 않으면 호출부에서 오류가 발생할 수 있습니다.

  2. Null 체크: update 메서드 내에서 startDate, endDate, finalApprovalDate, bnsManager 등의 값이 null인지 여부를 체크하는 로직이 필요할 수 있습니다. 특히 이러한 값들이 데이터베이스에 저장된 경우, null 값을 허용하지 않는 경우도 있으므로 오류가 발생할 수 있습니다.

  3. 테스트 커버리지: 이 변경 사항과 관련된 테스트 케이스가 충분한지 확인해야 합니다. 특히 매개변수가 삭제된 부분과 관련하여 기존의 기능이 정상적으로 작동하는지를 검증할 필요가 있습니다.

  4. 주석 추가: 코드의 변경 이유에 대한 주석을 추가하는 것도 좋습니다. 이는 다른 개발자들이 변경 사항을 이해하는 데 큰 도움이 됩니다.

  5. 코드 스타일: 코드 포매팅을 일관되게 유지하는 것이 좋습니다. 예를 들어, 각 메서드와 매개변수 간의 공백이 일관되게 유지된다면 가독성이 향상됩니다.

이러한 점들을 고려하여 코드를 개선해 나가면 더욱 안정적인 결과를 얻을 수 있을 것입니다.

Expand Down
6 changes: 3 additions & 3 deletions Admin/src/main/java/com/seveneleven/project/service/AdminProjectServiceImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,11 +68,11 @@ public Project updateProject(Long id, PutProject.Request request) {
}
}

Company customer = adminCompanyReader.getCompany(request.getCustomerId());
Company developer = adminCompanyReader.getCompany(request.getDeveloperId());
// Company customer = adminCompanyReader.getCompany(request.getCustomerId());
// Company developer = adminCompanyReader.getCompany(request.getDeveloperId());
ProjectType projectType = adminProjectTypeReader.getProjectType(request.getProjectTypeId());

Project updatedProject = request.updateProject(project, customer, developer, projectType);
Project updatedProject = request.updateProject(project, projectType);
return adminProjectStore.store(updatedProject);
}

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. customer와 developer 변수를 생성하는 코드를 주석 처리했습니다.
  2. updateProject 메서드 호출에서 customer와 developer를 제거하고 projectType만 전달하도록 수정했습니다.

잠재적인 버그 위험:

  1. 고객 및 개발자 정보 없음: customer와 developer 관련 정보가 주석 처리됨에 따라, 이후 로직에서 이 정보가 필요한 경우 NullPointerException이 발생할 수 있습니다. 특히 updateProject 메서드가 고객과 개발자 정보를 필요로 하는 경우 문제가 될 수 있습니다. 이 부분에서 예외 처리가 필요할 수 있습니다.

  2. 기존 로직에 대한 이해 부족: 주석 처리된 코드가 왜 필요하지 않은지, 혹은 어떤 이유로 제거했는지에 대한 설명이 부족합니다. 이는 나중에 코드를 유지보수하는 다른 개발자에게 혼란을 줄 수 있습니다.

개선 제안:

  1. 주석에 이유 추가: 고객 및 개발자 정보를 주석 처리한 이유를 코드에 명확하게 주석으로 남기면 향후 유지보수 시 이해하는 데 도움이 될 것입니다.

  2. 유효성 검사 추가: projectType 외에도 필요한 정보가 있는지(예: 고객 및 개발자) 확인하는 유효성 검사 로직을 추가하는 것이 좋습니다. 이는 데이터 무결성을 보장하는 데 도움이 됩니다.

  3. 명확한 메서드 시그니처: request.updateProject 메서드의 시그니처도 적절히 수정되었는지 확인해야 합니다. 인수와 반환 값이 변경된다면 해당 메서드를 호출하는 모든 코드도 업데이트해야 합니다.

  4. 리팩토링 고려: request.updateProject가 필요한 모든 정보를 통합해서 수정하는 것이 더 나을 경우, 요청 객체를 조정하여 필요한 정보를 모두 포함하게 할 수도 있습니다. 이를 통해 메서드 호출 시 적절한 정보를 전달할 수 있게 할 수 있습니다.

이러한 점들을 반영하면 코드의 안정성과 가독성을 높일 수 있습니다.

Expand Down
4 changes: 0 additions & 4 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 @@ -124,14 +124,10 @@ public Project update(
LocalDate startDate,
LocalDate endDate,
LocalDateTime approvalDate,
Company customer,
Company developer,
ProjectType ProjectType,
String bnsManager
) {
this.projectName = name;
this.customer = customer;
this.developer = developer;
this.projectDescription = description;
this.projectType = ProjectType;
this.projectStatusCode = statusCode;
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. 변수 삭제: Company customer와 Company developer 절이 제거되었습니다. 이 부분이 실제 비즈니스 로직에 중요한 변수라면 이들이 필요 없어졌는지 확인해야 합니다. 만약 불필요하다면 괜찮지만, 기존 코드에서 의존하고 있던 부분은 없는지 확인하는 것이 필요합니다.

  2. 일관성: 변수 이름이 ProjectType과 같이 대문자로 시작하는데, 이는 일반적으로 클래스나 인터페이스의 이름에 사용하는 방식입니다. Java에서 변수를 정의할 때는 소문자로 시작하는 것이 일반적이므로 projectType으로 변경하는 것이 좋습니다.

  3. 매개변수 이름: 코드의 가독성을 높이기 위해 매개변수의 이름을 더 명확하게 설정할 수 있습니다. 예를 들어, bnsManager 대신 businessManager와 같이 풀어서 의미를 명확히 하는 것도 고려해볼 수 있습니다.

  4. Null 검사: 만약 name, description, statusCode와 같은 매개변수들이 null이 될 경우 코드에서 오류가 발생할 수 있습니다. 입력 값에 대한 검증이 필요합니다.

  5. 주석 추가: 코드의 변경 사항이나 의도를 명확히 하기 위해 간단한 주석을 추가하는 것도 좋은 방법입니다. 예를 들어 왜 고객과 개발자 정보가 필요 없어졌는지 설명할 수 있습니다.

위의 사항들을 고려하여 수정하면 코드의 품질과 안정성을 높일 수 있을 것입니다.

Expand Down
2 changes: 1 addition & 1 deletion Common/src/main/java/com/seveneleven/entity/project/constant/MemberType.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
@Getter
@AllArgsConstructor
public enum MemberType {
ADMIN("admin"),
ADMIN("ADMIN"),
CLIENT("client"),
DEVELOPER("developer");

Copy link

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

Choose a reason for hiding this comment

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

코드 패치에 대한 간단한 리뷰입니다.

  1. 상수 표기법: 패치에서 ADMIN의 값을 "admin"에서 "ADMIN"으로 변경한 것은 좋습니다. 열거형 상수는 일반적으로 대문자로 표현하는 것이 관례이므로, 일관성을 유지하는 데 도움이 됩니다.

  2. 버그 리스크: 현재 코드에는 즉각적인 버그 위험은 없어 보입니다. 그러나 이 변경이 기존 코드와 호환성이 있는지 확인해보아야 합니다. 만약 이 enum 값을 사용하는 다른 코드가 "admin"을 기대하고 있다면, 잠재적인 오류를 초래할 수 있습니다. 따라서 전체 프로젝트에서 이 값을 참조하는 부분을 점검하는 것이 필요합니다.

  3. 문서화 및 주석: 열거형 타입이 어떤 목적으로 사용되는지에 대한 주석이 없다면, 코드를 사용하는 다른 개발자들이 이해하는 데 어려움을 겪을 수 있습니다. 각 타입의 용도와 그에 대한 간단한 설명을 주석으로 추가하는 것이 좋습니다.

  4. 일관성 유지: 다른 enum 값들도 대문자 형태로 통일해 주면 가독성이 높아질 수 있습니다. 예를 들어 CLIENT와 DEVELOPER도 대문자로 변경하는 것이 좋습니다.

  5. 추가적인 타입: 만약 나중에 더 많은 사용자 유형이 추가될 가능성이 있다면, 현재 지정된 MemberType의 구조를 점검하고, 확장성 있게 설계해 둘 필요가 있습니다.

종합적으로 보았을 때 이 변경은 긍정적이지만, 기존 코드와의 호환성 체크 및 코드 문서화에 유의해야 할 필요가 있습니다.

Expand Down
23 changes: 16 additions & 7 deletions Main/src/main/java/com/seveneleven/project/dto/GetProjectStep.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
import lombok.NoArgsConstructor;

import java.time.LocalDateTime;
import java.util.ArrayList;
import java.util.List;

public class GetProjectStep {
Expand All @@ -16,7 +17,7 @@ public class GetProjectStep {
@NoArgsConstructor
public static class Response {
private Long projectId;
private List<ProjectStepInfo> projectStepInfo;
private List<ProjectStepInfo> projectStepInfo = new ArrayList<>();

@Override
public String toString() {
Expand All @@ -25,9 +26,17 @@ public String toString() {
'}';
}

public Response(Long projectId, List<ProjectStepInfo> projectStepInfo) {
private Response(Long projectId) {
this.projectId = projectId;
this.projectStepInfo = projectStepInfo;
this.projectStepInfo = new ArrayList<>();
}

public static Response create(Long projectId) {
return new Response(projectId);
}

public void add(ProjectStep projectStep, List<Checklist> checklists) {
projectStepInfo.add(ProjectStepInfo.toDto(projectStep, projectStepInfo.size() + 1, checklists));
}
}

Expand All @@ -46,15 +55,15 @@ public String toString() {
'}';
}

private ProjectStepInfo(ProjectStep projectStep, List<Checklist> projectChecklist) {
private ProjectStepInfo(ProjectStep projectStep, Integer order, List<Checklist> projectChecklist) {
stepId = projectStep.getId();
stepName = projectStep.getStepName();
stepOrder = projectStep.getStepOrder();
stepOrder = order;
this.projectChecklist = projectChecklist.stream().map(ProjectChecklist::new).toList();
}

public static ProjectStepInfo toDto(ProjectStep projectStep, List<Checklist> projectChecklist) {
return new ProjectStepInfo(projectStep, projectChecklist);
public static ProjectStepInfo toDto(ProjectStep projectStep, Integer order, List<Checklist> projectChecklist) {
return new ProjectStepInfo(projectStep, order, projectChecklist);
}
}

Copy link

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

Choose a reason for hiding this comment

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

코드 리뷰에 대해 몇 가지 피드백을 드리겠습니다.

  1. 변경 사항 명확성: ProjectStepInfo 클래스의 생성자에 Integer order 매개변수를 추가한 것은 명확하게 보입니다. 그러나 코드의 다른 부분에서 order 값이 어떻게 사용되는지, 혹은 잘못된 값이 전달될 경우 발생할 수 있는 문제를 고려할 필요가 있습니다. 예를 들어, order가 null일 때의 처리를 추가하는 것이 좋습니다.

  2. 정확한 데이터 타입 사용: 현재 order가 Integer 타입인데, 만약 순서를 반드시 0 이상의 정수로 제한해야 한다면 Integer 대신 int를 사용하거나 유효성을 검증하는 로직을 구현하는 것이 필요합니다.

  3. 가독성: 코드의 가독성을 높이기 위해, 가능하다면 매개변수의 의미를 명확하게 색깔을 입혀주는 것도 좋습니다. 예를 들어, order 대신 더 구체적인 이름을 사용하는 것이 코드를 읽는 사람에게 도움이 될 수 있습니다.

  4. 예외 처리: order 값이 유효하지 않은 경우 (예: 음수인 경우 등) 어떻게 대처할 것인지에 대한 로직이 필요합니다. 이 부분에 대한 예외 처리를 추가하는 것이 좋습니다.

  5. 불변성: projectChecklist가 Immutable 컬렉션으로 전달되고 처리되는지 확인하십시오. mutable한 리스트가 전달될 경우 원본 데이터가 변경될 수 있으므로, 이를 방지하기 위한 방법을 고려해야 합니다.

결론적으로, 이 코드 패치는 기본적인 의도는 명확하지만, 입력 값에 대한 유효성 검사 및 예외 처리를 보완하면 더욱 안전하고 견고한 코드가 될 것입니다.

Expand Down
16 changes: 7 additions & 9 deletions Main/src/main/java/com/seveneleven/project/service/ProjectStepServiceImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,15 +39,13 @@ public GetStepChecklist.Response getStepChecklist(Long stepId) {
public GetProjectStep.Response getProjectStep(Long projectId) {
List<ProjectStep> projectSteps = stepReader.getProjectStep(projectId);

List<GetProjectStep.ProjectStepInfo> projectStepInfos = projectSteps.stream().map(stepInfo -> {
List<Checklist> checklists = checklistReader.read(stepInfo.getId());
return GetProjectStep.ProjectStepInfo.toDto(
stepInfo,
checklists
);
}).toList();

return new GetProjectStep.Response(projectId, projectStepInfos);
GetProjectStep.Response response = GetProjectStep.Response.create(projectId);

projectSteps.forEach(stepInfo -> {
response.add(stepInfo, checklistReader.read(stepInfo.getId()));
});

return response;
}
Comment on lines +42 to 49
Copy link
Collaborator

@K-KY K-KY Feb 18, 2025

Choose a reason for hiding this comment

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

image


@Override
Expand Down
Loading