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

섹션 5, 6 문제 풀이 #9

Merged
merged 2 commits into from
Dec 10, 2024
Merged

Conversation

JiHoon-0330
Copy link
Collaborator

파일별로 봐주시면 될 것 같습니다

  • jihoon/README.md 파일에 섹션/문항 별로 정리했습니다.
  • 파일 이름은 /섹션/문항.md 입니다.

@wwwr-kim0en
Copy link
Owner

wwwr-kim0en commented Nov 28, 2024

문제점을 찾은 것 같습니다. 제가 지금 PR을 올린 상태라 main 브랜치가 업데이트가 불가능해 코멘트로 남깁니다.
.github 디렉토리 > workflows 디렉토리 > set_reviewers.yml 을 다음과 같이 수정해주세요.

name: Auto Assign Reviewers

on:
  pull_request:
    types: [opened, ready_for_review]

permissions:
  pull-requests: write # PR 관련 권한 추가
  contents: read # 코드 내용 읽기 권한 추가

jobs:
  assign-reviewers:
    runs-on: ubuntu-latest
    steps:
      - name: Auto Assign Reviewers
        uses: actions/github-script@v6
        with:
          github-token: ${{ secrets.GITHUB_TOKEN }}
          script: |
            const reviewers = [
              'wwwr-kim0en',
              'kwonjounghun',
              'nagyum',
              'onblana',
              'NonamedBread',
              'JiHoon-0330',
              'JANGSEYEONG'
            ];

            // PR 작성자 제외
            const prAuthor = context.payload.pull_request.user.login;
            const availableReviewers = reviewers.filter(reviewer => reviewer !== prAuthor);

            // 랜덤하게 3명 선택
            const shuffled = availableReviewers.sort(() => 0.5 - Math.random());
            const selectedReviewers = shuffled.slice(0, 3);

            // 리뷰어 설정
            await github.rest.pulls.requestReviewers({
              owner: context.repo.owner,
              repo: context.repo.repo,
              pull_number: context.payload.pull_request.number,
              reviewers: selectedReviewers
            });


for (const c of num2Str) {
if (!num1Counter[c]) return false;
num1Counter[c]--;
Copy link
Owner

Choose a reason for hiding this comment

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

증감연산자(++,--)이 eslint에서는 지양하는 코드 스타일 중 하나라고 합니다. (https://eslint.org/docs/latest/rules/no-plusplus)

상관없다고 하는 개발자들도 있고, 꺼려하는 개발자들도 있는데, 왜 이렇게 다를까 찾아보니 아래와 같은 이유들이 있는 것 같습니다.

  1. 증감 연산자는 자동으로 세미콜론;이 추가되는 대상이 되어서, 예상치 못하게 코드의 흐름을 변경시키며, 의도하지 않은 값의 증가, 감소를 일으키는 등 애플리케이션 내에서 예상치 못한 에러를 발생시킬 수 있다고 합니다.
  2. (후위연산자,전위연산자) 연산이 먼저인지, 할당이 먼저인지 파악하는 것이 번거롭다고 합니다.

전 특별히 증감연산자가 꺼려지는 편은 아니지만, 그럼에도 불구하고 이런 관점도 알고 있으면 좋을 것 같다고 생각해서 리뷰를 남겨봅니다.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

우선 저는 한 줄에서 해당 연산만 진행하기 때문에 증감연산자를 사용한다고 해서 복잡하거나 가독성이 떨어진다고 생각하지는 않습니다.
작성해주신 증감연산자를 좋아하지 않는 이유는 여러가지 연산을 처리해야하는 상대적으로 복잡한 상황에 해당할 것 같습니다.

추가적으로 저는 제품을 만드는 코드와 알고리즘을 위한 코드 스타일이 같아야 한다고 생각하지는 않습니다.
물론 상황에 따라 지원하는 회사가 알고리즘 문제의 코드 스타일을 본다거나, 코딩테스트 문제에서도 함수를 여러개 분리해야 하는 복잡한 코드를 작성해야 한다면, 변수명과 같은 부분들이 중요할 수 있다고 생각합니다.

저는 이 영상을 보고 이런 생각을 가지게 되었는데 한 번 보셔도 좋을 것 같습니다.
https://youtu.be/bpXiz5ruXr4?t=21

Copy link
Owner

Choose a reason for hiding this comment

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

오호 저는 당연하게 프로젝트에서 작성하는 코드 스타일이 코테에도 적용된다고 생각했는데, 영상을 보니, 확실히 무슨 말이신지 이해했습니다! 감사합니다 ㅎ


let l = 0;
let record = {};
let max = -Infinity;
Copy link
Owner

Choose a reason for hiding this comment

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

혹시 -Infinity 로 초기값을 설정하신 이유가 있으실까요 ? 인강에서도 이렇게 하고있지만 그 이유가 잘 이해가 안 갔어서, 혹시 이유가 있으실지 궁금합니다.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

해당 문항에서는 문자열의 길이를 구하기 때문에 음수 값이 할당될 수 없으니 0 으로 초기화를 해도 상관이 없습니다.
하지만 최대값을 구할 땐 특정 값과 기존 최대 값의 비교를 통해 최대 값을 갱신해 나가기 때문에 초기화 값은 다음에 어떤 값과 비교를 하더라도 더 작은 값 이어야 한다는 가정이 있기 때문에 -Infinity 로 설정하는 것이 편한 것 같습니다

@wwwr-kim0en wwwr-kim0en merged commit 576fbf7 into wwwr-kim0en:main Dec 10, 2024
1 check failed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants