Skip to content
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.

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

Refactor/53 #54

Merged
merged 2 commits into from
Nov 18, 2024
Merged

Refactor/53 #54

merged 2 commits into from
Nov 18, 2024

Conversation

Enble
Copy link
Contributor

@Enble Enble commented Nov 18, 2024

변경 사항

ex) 로그인 시, 구글 소셜 로그인 기능을 추가했습니다.

테스트 실행

  • 👍 Yes
  • 🙅 No, Because they are not neaded
  • 🤯 No, Because I needed help with writing tests

테스트 결과

ex) 베이스 브랜치에 포함되기 위한 코드는 모두 정상적으로 동작해야 합니다. 결과물에 대한 스크린샷, GIF, 혹은 라이브 데모가 가능하도록 샘플API를 첨부할 수도 있습니다.

+α Checklist

  • 자바 코드 컨벤션을 지키면서 프로그래밍했는가?
  • 한 메서드에 오직 한 단계의 들여쓰기(indent)만 허용했는가?
  • else 예약어를 쓰지 않았는가?
  • 3개 이상의 인스턴스 변수를 가진 클래스를 구현하지 않았는가? (가능하면 인스턴스 변수의 수를 줄이기 위해 노력한다.)
  • setter 없이 구현했는가?
  • 메소드의 인자 수를 제한했는가? (4개 이상의 인자는 허용하지 않는다. 3개도 가능하면 줄이기 위해 노력해 본다.)
  • 코드 한 줄에 점(.)을 하나만 허용했는가?
  • 메소드가 한가지 일만 담당하도록 구현했는가?
  • 클래스를 작게 유지하기 위해 노력했는가?

@Enble Enble self-assigned this Nov 18, 2024
@Enble Enble linked an issue Nov 18, 2024 that may be closed by this pull request
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

@@ -25,7 +21,7 @@ jobs:
- name: Set up JDK 17 (Amazon Corretto)
uses: actions/setup-java@v4
with:
# distribution: 'corretto' # 올바른 배포판 이름
# distribution: 'corretto' # 올바른 배포판 이름
distribution: 'temurin'
java-version: '17'

Choose a reason for hiding this comment

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

코드에서 잘못된 부분이나 수정이 필요한 부분, 리팩토링할 수 있는 부분에 대한 리뷰를 하겠습니다.

  1. 이벤트 트리거 변경:

    • push 이벤트가 주석 처리되어 있습니다. CI 프로세스의 목적에 따라 push 이벤트를 주석 해제하고 필요한 브랜치를 지정하는 것이 좋습니다. 예를 들어, maindevelop 브랜치에 대한 푸시를 트리거할 수 있도록 설정할 수 있습니다.
    - push:
        branches:
          - main
          - develop
  2. 주석 처리된 코드:

    • distribution: 'corretto' 주석 처리된 부분이 있습니다. 주석은 이해를 돕기 위한 설명에 활용하되, 필요 없는 주석은 제거하는 것이 좋습니다. 현재 코드에서는 불필요한 주석이 있어 코드의 가독성을 떨어뜨릴 수 있습니다.
  3. 버전 관리:

    • 사용하는 Java 버전과 배포판 (temurin)에 대한 설명을 추가하면 다른 개발자들이 코드를 이해하는 데 도움이 될 수 있습니다. 예를 들어 주석을 추가하여 왜 temurin을 선택했는지 설명하는 것이 좋습니다.
  4. 일관성 있는 코드 스타일:

    • YAML 파일에서 공백이나 들여쓰기를 일관되게 유지하는 것이 중요합니다. 현재 설정의 일관성을 위해 들여쓰기를 다시 확인할 필요가 있습니다.
  5. CI 이름:

    • CI 이름을 'PicTalk dev CI'로 변경한 것은 실제 프로젝트와 일치하는지 확인할 필요가 있습니다. 이렇게 변경하는 것이 좋을지 검토해야 합니다.

이와 같은 수정을 통해 코드의 가독성과 유지보수성을 높일 수 있습니다.

@@ -1,4 +1,4 @@
name: UMC Dev CI/CD # workflow 이름
name: PicTalk dev CD # workflow 이름

on: # 이벤트 설정
pull_request:

Choose a reason for hiding this comment

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

코드에서 검토할 부분과 개선할 사항을 아래에 나열하였습니다.

  1. 이름 변경:

    • 현재 이름이 "PicTalk dev CD"로 변경되었는데, 애플리케이션 또는 프로젝트의 목적을 더욱 명확히 나타낼 수 있도록 이름을 지정하는 것이 좋습니다. 예를 들어, "PicTalk Continuous Deployment"와 같이 더 구체적으로 수정할 수 있습니다.
  2. 주석:

    • 주석이 간단하고 이해하기 쉬운 것은 좋지만, 팀원들이나 다른 개발자들이 코드를 이해하기 쉽도록 좀 더 상세하게 작성하는 것이 좋습니다. 예를 들어, "workflow 이름" 대신 "CI/CD 워크플로우의 이름"이라고 적으면 더욱 명확합니다.
  3. 이벤트 설정:

    • on: pull_request: 부분 이후에 이벤트에 대한 자세한 설명이나 특정 조건들이 추가되지 않아서, 향후 발전성을 고려하여 다양한 이벤트(예: push, release)를 추가하는 것이 좋습니다. 예를 들어:
      on:
        pull_request:
          types: [opened, synchronize, reopened]
        push:
          branches: [main]
  4. 문법 및 포맷:

    • YAML 파일에서 들여쓰기가 중요하므로 일관되게 유지해야 합니다. 만약 이후에 변수를 추가하거나 더 많은 설정을 추가할 경우, 일관된 방향으로 작성하도록 주의해야 합니다.

이와 같은 수정 및 개선을 통해 코드의 가독성과 유지보수성을 높일 수 있습니다.

@@ -7,7 +7,7 @@ spring:

jpa:
hibernate:
ddl-auto: create-drop
ddl-auto: update
show-sql: true
properties:
hibernate:

Choose a reason for hiding this comment

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

코드에서 잘못된 부분이나 수정이 필요한 부분, 그리고 리팩토링할 수 있는 부분을 검토한 결과는 다음과 같습니다:

  1. DDL Auto 설정 변경:

    • ddl-auto: create-drop은 개발 환경에서 테스트할 때 유용하지만, 프로덕션 환경에서는 데이터가 잃어버릴 수 있습니다. update로 바뀌었으나, 만약 운영 환경에서는 별도로 데이터 마이그레이션을 관리하는 것이 좋습니다. 따라서, 실제 프로덕션 환경에서는 none으로 설정하고, Liquibase나 Flyway와 같은 도구를 사용하여 데이터베이스 스키마 마이그레이션을 관리하는 것을 추천합니다.
  2. show-sql:

    • show-sql: true는 SQL 쿼리를 콘솔에 출력하는데 유용할 수 있으나, 프로덕션 환경에서는 성능 문제나 보안상의 이유로 false로 설정하는 것이 좋습니다. 필요한 경우 로깅 프레임워크를 통해 로그 레벨을 조정할 수 있습니다.
  3. Indentation 및 코드 포맷팅:

    • properties 아래의 인덴트가 올바르지 않습니다. YAML 문법에서는 계층 구조에 따라 들여쓰기가 중요하므로, 올바르게 맞춰줘야 합니다.

이러한 점들을 보완하여 다음과 같이 수정할 수 있습니다:

spring:
  jpa:
    hibernate:
      ddl-auto: none # 프로덕션 환경에서는 none으로 설정
    show-sql: false # 프로덕션 환경에서는 false로 설정
    properties:
      hibernate:
        # 필요 시 hibernate의 추가 속성 설정

추가적인 개선 사항으로, hibernate에 필요한 속성이나 동작을 명시적으로 추가하여, 설정 파일의 가독성을 높이거나 명확하게 하는 것도 좋은 방법입니다.

Copy link
Member

@lsh1215 lsh1215 left a comment

Choose a reason for hiding this comment

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

Good!

@Enble Enble merged commit 43cfd51 into develop Nov 18, 2024
2 checks passed
@Enble Enble deleted the refactor/53 branch November 18, 2024 06:16
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.

[refactor] 돈나가면 안돼
2 participants