Skip to content

Conversation

@Eseas
Copy link
Collaborator

@Eseas Eseas commented Feb 19, 2025

바로 MERGE하지 마세요! 검토 후 MERGE해야합니다!

✅ 최근 작업 주제 (하나 이상의 주제를 선택해주세요.)

  • 기능 추가
  • 리뷰 반영 수정
  • 리팩토링
  • 버그 수정
  • 컨벤션 수정
  • 브랜치 최신화

🏆 구현 목표

ex)
bucket post 및 bucket list view 초기 구현


📋 구현 사항 설명 (작업한 내용을 상세하게 기록합니다.)

ex)

  1. bucket post 기본 기능 구현
  2. bucket list view 기본 기능 구현

🔍 테스트 방법 (변경 사항을 확인하기 위한 테스트 방법을 기술합니다.)

ex)

  1. npm test 실행
  2. Postman으로 API 호출 테스트

🛠️ 쓰이는 모듈

ex)

  • corsheaders
  • jwt

💬 기타 질문 및 특이 사항

ex) 백프 배포 nginx 검토

Copy link

@github-actions github-actions bot left a 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

if(authorization.isPresent() && Objects.equals(APPROVER.toUpperCase(), authorization.get().getAuthorization())) {
return true;
}
log.error("memberId : {} not have authorization in project : {}", memberId, projectId);

Choose a reason for hiding this comment

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

코드 패치를 리뷰한 결과는 다음과 같습니다:

  1. 버그 위험:

    • authorization.get().getAuthorization()이 반환하는 값의 대소문자를 구분하지 않기 때문에, APPROVER를 모두 대문자로 변환하여 비교하는 것은 좋습니다. 하지만, authorization.get().getAuthorization() 값의 대소문자가 섞여 있을 경우, 원하는 결과가 아닐 수 있습니다. 값이 null일 경우에 대한 판단이 필요합니다.
  2. 성능 문제:

    • authorization.get()을 두 번 호출하고 있습니다. 이를 한 번만 호출하여 변수에 저장한 후 사용하는 것이 성능적으로 더 효율적입니다.
  3. 가독성:

    • Objects.equals() 사용은 null 체크를 포함하고 있어 안전하지만, 명시적인 null 체크를 추가하면 코드의 가독성을 높일 수 있습니다.
  4. 로깅:

    • 에러 로깅 메시지가 충분하지 않을 수 있습니다. 추가적인 정보(예: authorization 레이블 등)를 주면 문제 해결 시 유용할 겁니다.

개선한 코드는 다음과 같이 작성할 수 있습니다:

public boolean checkApprover(Long memberId, Long projectId) {
    Optional<Authorization> authorization = projectAuthorizationRepository
            .findByProjectIdAndMemberIdAndIsActive(projectId, memberId, YesNo.YES);
    
    if (authorization.isPresent()) {
        String authCode = authorization.get().getAuthorization();
        if (authCode != null && APPROVER.equals(authCode.toUpperCase())) {
            return true;
        }
    }
    
    log.error("memberId : {} does not have authorization in project : {}", memberId, projectId);
    return false;
}

위와 같이 코드를 수정함으로써 가독성, 성능, 안전성을 향상시킬 수 있습니다.

@Eseas Eseas merged commit 96da47b into multi-dev Feb 19, 2025
6 checks passed
@Eseas Eseas deleted the fix/authorize-if-fix branch February 19, 2025 05:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants