Skip to content

Conversation

@Eseas
Copy link
Collaborator

@Eseas Eseas commented Feb 19, 2025

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

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

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

🏆 구현 목표

  • authorize if 분기 조건 수정

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

  • 스웨거 로컬 테스트

🛠️ 쓰이는 모듈

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

company == (companyCode.equals(CUSTOMER.toLowerCase()) ? project.getCustomer() : project.getDeveloper())
) {
return true;
}

Choose a reason for hiding this comment

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

코드 패치에 대한 간단한 리뷰를 진행하겠습니다. 일부 버그 리스크와 개선 사항도 언급하겠습니다.

코드 리뷰

  1. null 체크: authorizationCodeauthorization.get().getAuthorization() 비교 전에 authorizationCode가 null인지 체크하는 것이 좋습니다. 현재 코드에서는 equals() 메소드를 호출하기 전에 authorizationCode가 null인 경우 NullPointerException이 발생할 수 있습니다. Objects.equals()를 사용하고 있지만, 입력 파라미터에 대한 유효성 확인이 선행되어야 합니다.

    if (authorizationCode != null &&
        Objects.equals(authorizationCode.toUpperCase(), authorization.get().getAuthorization()) &&
        company == ...
  2. 상수 사용: CUSTOMER를 직접 사용하기보다, CUSTOMER.toLowerCase()를 사용하는 것이 상수의 일관성을 해칠 수 있습니다. CUSTOMER 변수를 대문자로 통일하여 사용하는 것이 좋습니다. 또한, 비교 대상을 추가로 상수화하여 가독성을 높일 수 있습니다.

  3. 비교 연산자: company == 구문에서 객체의 동일성을 비교하고 있네요. 이 경우 ==가 아닌 .equals()를 사용하여 값의 동등성을 비교하는 것이 더 적절할 수 있습니다.

    company.equals(...)
  4. Optional 사용: 현재 코드에서는 authorization이 존재하는지 확인한 후에 값을 가져오고 있습니다. Optional을 적절히 사용하여 불필요한 null 체크를 최소화하도록 개선할 수 있습니다. authorization.ifPresentOrElse(...) 등을 활용할 수 있습니다.

  5. 예외 처리: AccessDeniedException이 발생할 수 있는 상황을 명확히 하고, 예외 발생 시 어떤 클라이언트에게 어떤 메시지를 반환할 것인지 고려해야 합니다. 디버깅과 오류 추적을 쉽게 하기 위해 로그를 기록하는 것도 좋은 접근입니다.

요약

코드의 전반적인 흐름은 좋은 것 같지만, null 체크와 상수 사용, 비교 연산자를 보다 신중하게 사용하여 버그 발생 가능성을 줄일 수 있습니다. Optional을 적극 활용하고, 예외 처리 방법에 대해 검토하면 더욱 안정적인 코드가 될 것입니다.

@Eseas Eseas merged commit 5433896 into multi-dev Feb 19, 2025
6 checks passed
@Eseas Eseas deleted the fix/authorize-if-fix branch February 19, 2025 04:35
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